Skip to content

ref(core)!: Remove Scope type interface in favor of using Scope class #14721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 16, 2024

In v8, types have been exported from @sentry/types, while implementations have been exported from other classes.
This lead to some duplication, where we had to keep an interface in @sentry/types, while the implementation mirroring that interface was kept e.g. in @sentry/core.
Since in v9 the types have been merged into @sentry/core, we can get rid of some of this duplication. This means that certain things that used to be a separate interface, will not expect an actual instance of the class/concrete implementation. This should not affect most users, unless you relied on passing things with a similar shape to internal methods.

This PR removes the Scope interface, in favor of just using the scope class everywhere.

This is related to #9840

@mydea mydea requested review from lforst and s1gr1d December 16, 2024 08:47
@mydea mydea self-assigned this Dec 16, 2024
Copy link
Contributor

github-actions bot commented Dec 16, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.22 KB -0.06% -12 B 🔽
@sentry/browser - with treeshaking flags 21.88 KB -0.06% -13 B 🔽
@sentry/browser (incl. Tracing) 35.74 KB -0.02% -4 B 🔽
@sentry/browser (incl. Tracing, Replay) 73 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.38 KB -0.04% -24 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas) 77.4 KB +0.01% +1 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 89.78 KB - -
@sentry/browser (incl. Feedback) 39.98 KB -0.03% -11 B 🔽
@sentry/browser (incl. sendFeedback) 27.83 KB -0.01% -2 B 🔽
@sentry/browser (incl. FeedbackAsync) 32.61 KB -0.03% -9 B 🔽
@sentry/react 25.93 KB -0.02% -5 B 🔽
@sentry/react (incl. Tracing) 38.6 KB -0.02% -6 B 🔽
@sentry/vue 27.46 KB -0.02% -5 B 🔽
@sentry/vue (incl. Tracing) 37.61 KB -0.02% -5 B 🔽
@sentry/svelte 23.39 KB -0.05% -10 B 🔽
CDN Bundle 24.38 KB -0.03% -7 B 🔽
CDN Bundle (incl. Tracing) 37.44 KB -0.06% -20 B 🔽
CDN Bundle (incl. Tracing, Replay) 72.54 KB -0.01% -2 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) 77.91 KB -0.02% -14 B 🔽
CDN Bundle - uncompressed 71.62 KB -0.02% -12 B 🔽
CDN Bundle (incl. Tracing) - uncompressed 111 KB -0.02% -12 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed 225.05 KB -0.01% -12 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 238.26 KB -0.01% -12 B 🔽
@sentry/nextjs (client) 38.85 KB -0.02% -4 B 🔽
@sentry/sveltekit (client) 36.28 KB -0.03% -10 B 🔽
@sentry/node 162.85 KB -0.01% -12 B 🔽
@sentry/node - without tracing 99.01 KB -0.01% -8 B 🔽
@sentry/aws-serverless 126.74 KB -0.01% -5 B 🔽

View base workflow run

@mydea
Copy link
Member Author

mydea commented Dec 16, 2024

Hmm, some test failures still happening, I'll check it out.

Copy link

codecov bot commented Dec 16, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
595 1 594 134
View the top 1 failed tests by shortest run time
client-app-routing-instrumentation.test.ts Creates a navigation transaction for app router routes
Stack Traces | 30s run time
client-app-routing-instrumentation.test.ts:19:5 Creates a navigation transaction for app router routes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

mydea added a commit that referenced this pull request Dec 16, 2024

Unverified

The committer email address is not verified.
Extracted out from
#14721

Today, we inline all the types into `deno/build/index.d.ts`. This used
to work "OK" previously, where all the types came from `@sentry/types`
and where thus all "simple types".

If we want to replace some of this with actual classes, e.g. `Scope` or
`Client` classes, this starts to fail, because now we have separate
definitions that do not match.

So to fix this, we now stop inlining all the types, but instead
re-export them from `@sentry/core`. While at this, I also updated the
test setup to ignore utils/types and just use core for everything.

With this change, `deno/build/index.d.ts` looks something like this:

```ts
import * as _sentry_core from '@sentry/core';
import { BaseTransportOptions, Options, TracePropagationTargets, ClientOptions, ServerRuntimeClient, Integration, Client } from '@sentry/core';
export { ... } from '@sentry/core';

// additional types from deno go here...
```

I do not _think_ this should be breaking, but 🤷 hard to say for sure
with these things.
@mydea mydea force-pushed the fn/drop-scope-type branch from a16d7ef to a086fad Compare December 16, 2024 15:58
mydea and others added 2 commits December 16, 2024 17:55

Unverified

The committer email address is not verified.
…lass

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com>
@mydea mydea force-pushed the fn/drop-scope-type branch from dbe5f6c to 9f4204b Compare December 16, 2024 16:56

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mydea mydea merged commit c1d374d into develop Dec 17, 2024
158 checks passed
@mydea mydea deleted the fn/drop-scope-type branch December 17, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants