Skip to content

fix incorrect sampled type on Transaction#11115

Merged
Lms24 merged 2 commits intogetsentry:developfrom
quisido:patch-1
Mar 18, 2024
Merged

fix incorrect sampled type on Transaction#11115
Lms24 merged 2 commits intogetsentry:developfrom
quisido:patch-1

Conversation

@quisido
Copy link
Copy Markdown
Contributor

@quisido quisido commented Mar 15, 2024

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@sentry/types does not align with @sentry/core, and it causes build errors when exactOptionalPropertyTypes is true.

Example error below:

.../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction.d.ts:5:22 - error TS2420: Class 'import(".../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction").Transaction' incorrectly implements interface 'import(".../Yarn/Berry/cache/@sentry-types-npm-7.107.0-a994f84978-10c0.zip/node_modules/@sentry/types/types/transaction").Transaction'.
  Types of property 'sampled' are incompatible.
    Type 'boolean | undefined' is not assignable to type 'boolean'.
      Type 'undefined' is not assignable to type 'boolean'.

5 export declare class Transaction extends SpanClass implements TransactionInterface {
                       ~~~~~~~~~~~


Found 1 error in .../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction.d.ts:5

This change fixes this.

@Lms24
Copy link
Copy Markdown
Member

Lms24 commented Mar 15, 2024

Hey @quisido thanks for opening this PR! Which version are you experiencing this problem on?

Generally, we're going to remove sampled entirely from the SDK so merging the PR into our v8 (develop) branch won't make much sense. We could merge it into v7 though if this is currently causing issues for you on a v7 version of the SDK.

@quisido
Copy link
Copy Markdown
Contributor Author

quisido commented Mar 15, 2024

Hey @quisido thanks for opening this PR! Which version are you experiencing this problem on?

Generally, we're going to remove sampled entirely from the SDK so merging the PR into our v8 (develop) branch won't make much sense. We could merge it into v7 though if this is currently causing issues for you on a v7 version of the SDK.

Sorry, yes, v7.

@quisido
Copy link
Copy Markdown
Contributor Author

quisido commented Mar 16, 2024

@Lms24 I've opened a PR for v7 at #11146, but fwiw this bug would still need to be fixed on v8 via this PR, as upgrading to v8 would re-introduce the bug otherwise.

Lms24
Lms24 approved these changes Mar 18, 2024
View reviewed changes
Copy link
Copy Markdown
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.

I see, thx for pointing out that this is still a problem in v8 (at least at the moment). I missed that Transaction now inherits ultimately also from SpanContext where we defined the explicit undefined.

As I said, medium-term, we'll remove Transaction so this won't be an issue anymore but for now, let's ensure we fix this. Thanks!

@Lms24 Lms24 enabled auto-merge (squash) March 18, 2024 11:58
@Lms24 Lms24 merged commit ffdc8aa into getsentry:develop Mar 18, 2024
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
@quisido @cadesalaberry
fix(types): Fix incorrect sampled type on Transaction (getsentry#…
95f6f28 
…11115)

`@sentry/types` does not align with `@sentry/core`, and it causes build
errors when `exactOptionalPropertyTypes` is `true`.

This patch fixes the TypeScript error by explicitly adding `undefined` to the `sampled` flag of `Transaction`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24 Lms24 Lms24 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@quisido @Lms24