ref: Avoid optional chaining & add eslint rule#6777
Conversation
size-limit report 📦
|
Lms24
left a comment
There was a problem hiding this comment.
LGTM and nice bundle size improvement!
| }); | ||
| } | ||
|
|
||
| function getEventExceptionValues(event: Event): { type: string; value: string } { |
|
Just gonna link to #6459 as we're kinda tracking bundle size improvements there |
|
ref #6778 as that touches similar files (note for myself) |
7783de0 to
f11d97b
Compare
As this is transpiled to a rather verbose form.
| * Default: undefined | ||
| */ | ||
| _experiments?: Partial<{ enableLongTask: boolean; enableInteractions: boolean }>; | ||
| _experiments: Partial<{ enableLongTask: boolean; enableInteractions: boolean }>; |
There was a problem hiding this comment.
Just to double check here, is this change OK? It is actually always set right now because it is (as {}) part of the DEFAULT_BROWSER_TRACING_OPTIONS. By actually marking this non-optional we can save on checks throughout tracing a bit. cc @AbhiPrasad @Lms24
There was a problem hiding this comment.
I think this is fine because it's still optional in the integration constructor, and that is all that matters.
There was a problem hiding this comment.
Jup, this was my thought as well 👍
packages/vue/src/tracing.ts
Outdated
|
|
||
| vm.$_sentryRootSpanTimer = setTimeout(() => { | ||
| if (vm.$root?.$_sentryRootSpan) { | ||
| if (vm.$root.$_sentryRootSpan) { |
There was a problem hiding this comment.
Also here just to double check cc @Lms24 , type-wise $root cannot be undefined, not sure if the optional chaining guard was there for a different reason?
There was a problem hiding this comment.
Hmm not sure to be honest. With the type, you mean this here, correct?
sentry-javascript/packages/vue/src/types.ts
Lines 14 to 27 in 2aa4e94
Since it seems like we vendored this type from Vue, I wouldn't be too sure that $root always existed (older Vue versions...). According to this PR, we introduced it with Vue 3 support: #3804. I'd just check for vm.$root here anyway to make sure. Shouldn't hurt us too much in terms of bundle size. WDYT?
There was a problem hiding this comment.
jup, I will revert this then, to be safe!
There was a problem hiding this comment.
I checked, and as far as I can tell $root should be available in both vue2 and vue3. But, hard to be sure... so I reverted the check, to be on the safe side.
f11d97b to
0fb8ab6
Compare
packages/vue/src/tracing.ts
Outdated
|
|
||
| vm.$_sentryRootSpanTimer = setTimeout(() => { | ||
| if (vm.$root?.$_sentryRootSpan) { | ||
| if (vm.$root.$_sentryRootSpan) { |
There was a problem hiding this comment.
Hmm not sure to be honest. With the type, you mean this here, correct?
sentry-javascript/packages/vue/src/types.ts
Lines 14 to 27 in 2aa4e94
Since it seems like we vendored this type from Vue, I wouldn't be too sure that $root always existed (older Vue versions...). According to this PR, we introduced it with Vue 3 support: #3804. I'd just check for vm.$root here anyway to make sure. Shouldn't hurt us too much in terms of bundle size. WDYT?
packages/vue/src/tracing.ts
Outdated
| // Otherwise, retrieve the current span and finish it. | ||
| if (internalHook == internalHooks[0]) { | ||
| const activeTransaction = this.$root?.$_sentryRootSpan || getActiveTransaction(); | ||
| const activeTransaction = this.$root.$_sentryRootSpan || getActiveTransaction(); |
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
As this is transpiled to a rather verbose form.
This PR now also adds an eslint rule based on facebook/lexical#3233 to enforce avoiding optional chaining. I left it for all node-based stuff, because there we don't care, I guess.