feat(effect): Add base skaffolding for Effect.ts#19622
feat(effect): Add base skaffolding for Effect.ts#19622JPeer264 wants to merge 3 commits intojp/add-effect-sdkfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/effect/src/index.ts
Outdated
| } from '@sentry/core'; | ||
|
|
||
| export { EffectClient } from './client'; | ||
| export { init } from './sdk'; |
There was a problem hiding this comment.
Feat PR missing integration or E2E tests
Low Severity
This is a feat PR that introduces a new package (@sentry/effect) with exported init, EffectClient, and re-exported core APIs, but includes no integration or E2E tests. The review rules require that feat PRs include at least one integration or E2E test. Adding basic tests for init and EffectClient construction would help catch regressions early.
Triggered by project rule: PR Review Guidelines for Cursor Bot
There was a problem hiding this comment.
yes. That will come once implemented. They are already written, maybe not to perfection just yet.
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
|
|
||
| const MainLive = HttpLive.pipe( | ||
| Layer.provide( | ||
| Sentry.effectLayer({ |
There was a problem hiding this comment.
Do we export effectLayer? I only see init getting exported 🤔
There was a problem hiding this comment.
The README already reflects the actual usage, while the export doesn't exist yet, this will come in another PR (also init is exposed here, just for the sake of completeness)
This will be added in later steps.
packages/effect/src/sdk.ts
Outdated
| }), | ||
| transport: options.transport, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Don't forget to call applySdkMetadata with the needed arguments here.
There was a problem hiding this comment.
I removed sdk.ts entirely, as we don't need it.
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/1
branch
from
04bb301 to
86f7209
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| setMeasurement, | ||
| getSpanStatusFromHttpCode, | ||
| setHttpStatus, | ||
| } from '@sentry/core'; |
There was a problem hiding this comment.
Missing init export despite PR description claiming inclusion
Medium Severity
The PR description explicitly states "also init is exposed here, just for the sake of completeness" but init is neither defined nor exported anywhere in packages/effect/src/index.ts. Every other SDK package in this repo (nestjs, bun, svelte, react, etc.) defines its own init function that wraps initAndBind from @sentry/core and calls applySdkMetadata. Neither init nor applySdkMetadata exists in this package. A reviewer also flagged the missing applySdkMetadata call.
| it('has correct exports', () => { | ||
| expect(index.captureException).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No integration or E2E test for feat PR
Low Severity
Per the review rules, a feat PR needs at least one integration or E2E test. The only test present checks that captureException (a re-export from @sentry/core) is defined. It doesn't test any new behavior introduced by this package.
Triggered by project rule: PR Review Guidelines for Cursor Bot
| @@ -0,0 +1,52 @@ | |||
| # Sentry Effect SDK | |||
There was a problem hiding this comment.
l: Let's mark this alpha and follow our general structure for READMEs, e.g. https://github.com/getsentry/sentry-javascript/blob/develop/packages/tanstackstart-react/README.md#official-sentry-sdk-for-tanstack-start-react-alpha
And let's also add the notice below regarding potential breaking changes. Unlike our tss readme, I'd use Github's proper announcement boxes.
3 participants
This is one of many PRs to create the effect SDK. Once this has been merged I will open the draft PR for the effect sdk and create the plan in there.
(the almost final SDK can be viewed here: https://github.com/getsentry/sentry-javascript/tree/jp/effect-sdk. It might be that some specifics change, especially when having browser + server split, and with tracing)
This PR focuses on the base skaffolding of
@sentry/effect. This on its own is not really doing anything except setting up the skaffold. The README already reflects the actual usage, while the export doesn't exist yet, this will come in another PR (alsoinitis exposed here, just for the sake of completeness)