Skip to content
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

ref: Make span types more robust #10660

Merged
merged 3 commits into from
Feb 15, 2024
Merged

ref: Make span types more robust #10660

merged 3 commits into from
Feb 15, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Feb 14, 2024

With more strict TS settings, these could lead to problems:

  1. startChild() should return a span interface, to match the interface implementation.
  2. All properties that are optional in span/transactions need to also accept | undefined to be really correct - otherwise when we extend this and set undefined explicitly it complains there.

Fixes #10654

@mydea mydea requested review from lforst and Lms24 February 14, 2024 16:10
@mydea mydea self-assigned this Feb 14, 2024
Copy link
Contributor

github-actions bot commented Feb 14, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.82 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 69.06 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.98 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.67 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.25 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.16 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.16 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.16 KB (0%)
@sentry/browser - Webpack (gzipped) 22.43 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 76.11 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.64 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.6 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.71 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 212.88 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 101.5 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 74.01 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.83 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.43 KB (0%)
@sentry/react - Webpack (gzipped) 22.45 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 87.2 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.37 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 17.12 KB (0%)

@timfish
Copy link
Collaborator

timfish commented Feb 15, 2024

Oh wow, TS2416 is new to me 😂

If this fixes the issue, it's might be worth adding a test of some kind to ensure we don't break exactOptionalPropertyTypes anywhere else in the future!

@mydea
Copy link
Member Author

mydea commented Feb 15, 2024

TS2416

Yeah, it's a bit obscure and hard to reason about IMHO 😬 Let's see if that fixes it first, maybe we can opt-into this stricter setting in our TS E2E test 🤔

With more strict TS settings, these could lead to problems:

1. `startChild()` should return a span interface, to match the interface implementation.
2. no need to overwrite `updateName` for transaction, this messes with the inherited type `this` as the this is now a transaction, not a span... we can skip this.

Fixes #10654
@mydea
Copy link
Member Author

mydea commented Feb 15, 2024

OK, I actually tried this on the reproduction, and turns out the updateName thing was a bit misleading - it was only complaining because the transaction type was not "correct" because we had mismatches between types in interface and in the classes. Now, I made all optional properties in span/transaction accept xxx?: string | undefined, to ensure this is actually correct. The repro built for me then!

@ansf
Copy link

ansf commented Feb 15, 2024

exactOptionalPropertyTypes makes

 sampled?: boolean;

and

sampled: boolean | undefined

incompatible.

This is more in-line with the actual javascript runtime (e.g.):

const x = { sampled: undefined }
assert("sampled" in x)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I guess this falls into the "if this is what we have to do then let's do it" category 😅

@mydea mydea merged commit afa5304 into develop Feb 15, 2024
97 checks passed
@mydea mydea deleted the fn/improve-span-types branch February 15, 2024 10:37
mydea added a commit that referenced this pull request Feb 21, 2024
With more strict TS settings, these could lead to problems:

1. `startChild()` should return a span interface, to match the interface
implementation.
2. All properties that are optional in span/transactions need to also
accept `| undefined` to be really correct - otherwise when we extend
this and set `undefined` explicitly it complains there.

Fixes #10654
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.

@sentry/browser version 7.93.0 and later does not build with typescript exactOptionalPropertyTypes
5 participants