-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(meta-sdks): Remove runtime tags #13105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear the tests will need updating but lgtm
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Just had a suggestion to remove some tests that specifically tested the runtime tag before.
|
||
expect(getIsolationScope().getScopeData().tags).toEqual({ runtime: 'browser' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -45,8 +45,6 @@ describe('Sentry server SDK', () => { | |||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({}); | |||
|
|||
init({ dsn: 'https://[email protected]/1337' }); | |||
|
|||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'node' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -36,7 +36,5 @@ describe('Initialize Solid Start SDK', () => { | |||
|
|||
it('sets the runtime tag on the isolation scope', () => { | |||
solidStartInit({ dsn: 'https://[email protected]/1337' }); | |||
|
|||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'node' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -36,7 +36,5 @@ describe('Initialize Solid Start SDK', () => { | |||
|
|||
it('sets the runtime tag on the isolation scope', () => { | |||
solidStartInit({ dsn: 'https://[email protected]/1337' }); | |||
|
|||
expect(SentrySolid.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'browser' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -48,7 +48,5 @@ describe('Client init()', () => { | |||
expect(SentryReact.getIsolationScope().getScopeData().tags).toEqual({}); | |||
|
|||
init({ dsn: 'https://[email protected]/1337' }); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -52,8 +52,6 @@ describe('Server init()', () => { | |||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({}); | |||
|
|||
init({ dsn: 'https://[email protected]/1337' }); | |||
|
|||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'node' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -77,8 +77,6 @@ describe('Client init()', () => { | |||
expect(SentryReact.getIsolationScope().getScopeData().tags).toEqual({}); | |||
|
|||
init({ dsn: 'https://[email protected]/1337' }); | |||
|
|||
expect(SentryReact.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'browser' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
||
expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'node' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: let's delete the entire test since this was the only relevant check. I don't think we need to assert that the isolation scope has no tags at the beginning for Astro specifically.
Runtime tags are already set by e.g. Relay, so the Meta-Framework SDKs do not need to set them anymore.
Comment why this change was made: #13101 (review)