Conversation
size-limit report 📦
|
packages/core/src/scope.ts
Outdated
|
|
||
| /** @inheritDoc */ | ||
| public getScopeData(): ScopeData { | ||
| public getPerScopeData(): ScopeData { |
There was a problem hiding this comment.
Does getPerScopeData have to be public API?
I also am not the biggest fan of the scope class being aware of a separate global scope. I'd rather it be the consumers of the scope that decide to use the global scope.
There was a problem hiding this comment.
Hmm, I see your point. I'll see if we can do this another way!
0ed9262 to
b17d167
Compare
| // of `@sentry/core` that does not have the `getAttachments` method. | ||
| // See: https://github.com/getsentry/sentry-javascript/issues/5229 | ||
| // Merge scope data together | ||
| const data = getGlobalScope().getScopeData(); |
There was a problem hiding this comment.
@AbhiPrasad I refactored this so that the global scope is merged/applied in prepareEvent.
| const data = getGlobalScope().getPerScopeData(); | ||
| const isolationScopeData = this._getIsolationScope().getPerScopeData(); | ||
| const scopeData = this.getPerScopeData(); | ||
| const globalScope = getGlobalScope(); |
There was a problem hiding this comment.
When we introduce the isolation scope in a follow up PR, we can clean this up here and drastically simplify this! But for now I think this is "OK".
AbhiPrasad
left a comment
There was a problem hiding this comment.
Thanks for making the change!
packages/core/src/globals.ts
Outdated
| import type { Scope } from '@sentry/types'; | ||
|
|
||
| interface GlobalData { | ||
| globalScope?: Scope; |
There was a problem hiding this comment.
m: we need to be careful for global{s}.ts because of #5969
Let's rename this file to globalscope.ts? Also why do we have the indirection to GLOBAL_DATA.globalScope, can't we point to globalScope directly? If we refactor to need more GLOBAL_DATA, items, we can add it back.
This scope lives in module scope and is applied to _all_ events.
b17d167 to
94ae54c
Compare
This scope lives in module scope and is applied to all events.
Please review this carefully, as it is important that data is correctly applied etc. There should be a decent amount of tests covering all of this, but just to make sure. This was mostly ported/extracted from node-experimental.