ref: Avoid optional chaining & add eslint rule #6777

mydea · 2023-01-16T08:51:38Z

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.

github-actions · 2023-01-16T09:00:51Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.82 KB (-0.11% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	61.44 KB (-0.04% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.49 KB (-0.62% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	54.74 KB (-0.48% 🔽)
@sentry/browser - Webpack (gzipped + minified)	20.22 KB (-0.75% 🔽)
@sentry/browser - Webpack (minified)	66.17 KB (-0.57% 🔽)
@sentry/react - Webpack (gzipped + minified)	20.24 KB (-0.78% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	47.57 KB (-0.21% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.78 KB (-0.18% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	25.11 KB (-0.69% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	43.11 KB (-1.55% 🔽)
@sentry/replay - Webpack (gzipped + minified)	38.45 KB (-1.47% 🔽)

Lms24

LGTM and nice bundle size improvement!

Lms24 · 2023-01-16T09:24:15Z

packages/replay/src/coreHandlers/handleGlobalEvent.ts

@@ -89,3 +87,11 @@ function addInternalBreadcrumb(arg: Parameters<typeof addBreadcrumb>[0]): void {
    ...rest,
  });
 }
+
+function getEventExceptionValues(event: Event): { type: string; value: string } {


Nice refactor!

Lms24 · 2023-01-16T09:35:04Z

Just gonna link to #6459 as we're kinda tracking bundle size improvements there

mydea · 2023-01-16T09:52:04Z

ref #6778 as that touches similar files (note for myself)

As this is transpiled to a rather verbose form.

mydea · 2023-01-16T12:29:41Z

packages/tracing/src/browser/browsertracing.ts

@@ -90,7 +90,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions {
   *
   * Default: undefined
   */
-  _experiments?: Partial<{ enableLongTask: boolean; enableInteractions: boolean }>;
+  _experiments: Partial<{ enableLongTask: boolean; enableInteractions: boolean }>;


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

I think this is fine because it's still optional in the integration constructor, and that is all that matters.

Jup, this was my thought as well 👍

mydea · 2023-01-16T12:32:14Z

packages/vue/src/tracing.ts

@@ -40,7 +41,7 @@ function finishRootSpan(vm: VueSentry, timestamp: number, timeout: number): void
  }

  vm.$_sentryRootSpanTimer = setTimeout(() => {
-    if (vm.$root?.$_sentryRootSpan) {
+    if (vm.$root.$_sentryRootSpan) {


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?

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

export type ViewModel = {

_isVue?: boolean;

__isVue?: boolean;

$root: ViewModel;

$parent?: ViewModel;

$props: { [key: string]: any };

$options?: {

name?: string;

propsData?: { [key: string]: any };

_componentTag?: string;

__file?: string;

};

};

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?

jup, I will revert this then, to be safe!

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.

packages/utils/src/requestdata.ts

Lms24 · 2023-01-16T13:32:28Z

packages/vue/src/tracing.ts

@@ -40,7 +41,7 @@ function finishRootSpan(vm: VueSentry, timestamp: number, timeout: number): void
  }

  vm.$_sentryRootSpanTimer = setTimeout(() => {
-    if (vm.$root?.$_sentryRootSpan) {
+    if (vm.$root.$_sentryRootSpan) {


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

export type ViewModel = {

_isVue?: boolean;

__isVue?: boolean;

$root: ViewModel;

$parent?: ViewModel;

$props: { [key: string]: any };

$options?: {

name?: string;

propsData?: { [key: string]: any };

_componentTag?: string;

__file?: string;

};

};

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?

Lms24 · 2023-01-16T13:33:16Z

packages/vue/src/tracing.ts

@@ -96,7 +97,7 @@ export const createTracingMixins = (options: TracingOptions): Mixins => {
        // Start a new span if current hook is a 'before' hook.
        // Otherwise, retrieve the current span and finish it.
        if (internalHook == internalHooks[0]) {
-          const activeTransaction = this.$root?.$_sentryRootSpan || getActiveTransaction();
+          const activeTransaction = this.$root.$_sentryRootSpan || getActiveTransaction();


Likewise here, I guess

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

mydea added the Package: replay Issues related to the Sentry Replay SDK label Jan 16, 2023

mydea requested review from billyvg and Lms24 January 16, 2023 08:51

mydea self-assigned this Jan 16, 2023

mydea mentioned this pull request Jan 16, 2023

feat(replay): Stop replays when a segment could not be sent #6765

Merged

AbhiPrasad approved these changes Jan 16, 2023

View reviewed changes

Lms24 approved these changes Jan 16, 2023

View reviewed changes

Lms24 mentioned this pull request Jan 16, 2023

[Replay] Refactors and Nice-to-have Improvements #6459

Closed

12 tasks

mydea changed the title ~~ref(replay): Avoid optional chaining~~ ref: Avoid optional chaining & add eslint rule Jan 16, 2023

mydea force-pushed the fn/replay-avoid-optional-chaining branch from 7783de0 to f11d97b Compare January 16, 2023 10:16

mydea added 4 commits January 16, 2023 13:22

ref(replay): Avoid optional chaining

0a6c583

As this is transpiled to a rather verbose form.

build: Add no-optional-chaining eslint rule

5a62562

ref: Avoid optional chaining

973d090

fix replay

8095f38

mydea commented Jan 16, 2023

View reviewed changes

minor pr fix

0fb8ab6

mydea commented Jan 16, 2023

View reviewed changes

mydea force-pushed the fn/replay-avoid-optional-chaining branch from f11d97b to 0fb8ab6 Compare January 16, 2023 12:33

mydea mentioned this pull request Jan 16, 2023

ref(tracing): Remove nullish coalescing operator and add eslint rule #6783

Merged

Lms24 approved these changes Jan 16, 2023

View reviewed changes

Apply suggestions from code review

751b5eb

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>

AbhiPrasad approved these changes Jan 16, 2023

View reviewed changes

guard for vue $root

ae72aac

mydea merged commit 36492ce into master Jan 16, 2023

mydea deleted the fn/replay-avoid-optional-chaining branch January 16, 2023 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Avoid optional chaining & add eslint rule #6777

ref: Avoid optional chaining & add eslint rule #6777

mydea commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading

Lms24 left a comment

Lms24 Jan 16, 2023

Lms24 commented Jan 16, 2023

mydea commented Jan 16, 2023

mydea Jan 16, 2023 •

edited

Loading

AbhiPrasad Jan 16, 2023

mydea Jan 16, 2023

mydea Jan 16, 2023

Lms24 Jan 16, 2023

mydea Jan 16, 2023

mydea Jan 16, 2023

Lms24 Jan 16, 2023

Lms24 Jan 16, 2023

	export type ViewModel = {
	_isVue?: boolean;
	__isVue?: boolean;
	$root: ViewModel;
	$parent?: ViewModel;
	$props: { [key: string]: any };
	$options?: {
	name?: string;
	propsData?: { [key: string]: any };
	_componentTag?: string;
	__file?: string;
	};
	};

ref: Avoid optional chaining & add eslint rule #6777

ref: Avoid optional chaining & add eslint rule #6777

Conversation

mydea commented Jan 16, 2023 • edited Loading

github-actions bot commented Jan 16, 2023 • edited Loading

size-limit report 📦

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Jan 16, 2023

mydea commented Jan 16, 2023

mydea Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Jan 16, 2023 •

edited

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading

mydea Jan 16, 2023 •

edited

Loading