Sentry-javascript: Breaking API change between patch update of @sentry/tracing 6.0.2 to 6.0.3

[r3c]

• [x] Review the documentation: https://docs.sentry.io/
• [x] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• [x] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
• [ ] Provide a link to the affected event from your Sentry account

Package + Version

• [x] @sentry/browser 6.0.2
• [x] @sentry/tracing 6.0.3

Version:

6.0.3

Description

Latest patch version bump of @sentry/tracing from 6.0.2 to 6.0.3 broke our CI due to a type incompatibility. It seems this is caused by #3192 that introduces new non-optional methods in exported type Span. As a consequence 6.0.3 is not compatible with version 6.0.2 of related packages (in our case @sentry/browser) and single package updates through Dependabot isn't possible anymore. Initializing Sentry with mixed 6.0.2 + 6.0.3 versions results in following compilation error:

Sentry.init({
  // ...
  integrations: [new Integrations.BrowserTracing()],
  // ...
});

src/index.tsx:18:20 - error TS2322: Type 'BrowserTracing' is not assignable to type 'Integration'.
  Types of property 'setupOnce' are incompatible.
    Type '(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub) => void' is not assignable to type '(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub) => void'.
      Types of parameters '_' and 'addGlobalEventProcessor' are incompatible.
        Types of parameters 'callback' and 'callback' are incompatible.
          Type 'import("C:/.../node_modules/@sentry/types/dist/eventprocessor").EventProcessor' is not assignable to type 'import("C:/.../node_modules/@sentry/browser/node_modules/@sentry/types/dist/eventprocessor").EventProcessor'.
            Types of parameters 'event' and 'event' are incompatible.
              Type 'import("C:/.../node_modules/@sentry/browser/node_modules/@sentry/types/dist/event").Event' is not assignable to type 'import("C:/.../node_modules/@sentry/types/dist/event").Event'.
                Types of property 'spans' are incompatible.
                  Type 'import("C:/.../node_modules/@sentry/browser/node_modules/@sentry/types/dist/span").Span[] | undefined' is not assignable to type 'import("C:/.../node_modules/@sentry/types/dist/span").Span[] | undefined'.
                    Type 'import("C:/.../node_modules/@sentry/browser/node_modules/@sentry/types/dist/span").Span[]' is not assignable to type 'import("C:/.../node_modules/@sentry/types/dist/span").Span[]'.
                      Type 'Span' is missing the following properties from type 'Span': toContext, updateWithContext

18     integrations: [new Integrations.BrowserTracing()],
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Interface Span is a public exported interface shared between multiple packages, therefore adding non-optional methods to it should probably be considered as a major version change instead of patch to comply with semver.

[Sija]

So interesting to see how unstable sentry-javascript is 🍿 . In raven-js OTOH breaking changes or otherwise serious bugs were happening once every n versions/months as far as I remember, here it seems there's no stable version since the beginning of this (rewritten) lib. Really, really interesting...

[kamilogorek]

single package updates through Dependabot isn't possible anymore

It was never possible. We have a strict version requirement in package.json file of every package we release.
We don't use carrets nor tildes there for a reason, and package resolution should correctly reflect that. If not, then it means that lock files are incorrectly generated and should be purged and rebuilt.

@Sija you are comparing a rather small raven-js package, which was capable mostly of sending errors and messages, written in raw es5, to SDK written in TypeScript, which has 10x more features and complexity. There's also a difference between runtime stability vs. type issues which can be temporarily solved by pinning version and have no impact on the end-user of an app.

[Sija]

@kamilogorek First versions of sentry-javascript weren't that far from raven-js feature-wise, now perhaps it's a different story (well, years have passed...). In regards to the stability, breaking builds - as well as runtime errors - in almost every version - are something that makes using this lib a bumpy ride, I think it's pretty obvious. Wrongly implemented breadcrumb handler was there since very early days - and this faulty implementation was hogging the users' browsers - not sth you would expect from a error reporting library to say the least :D

[clinejj]

I'm getting a new issue as well on CI (not seeing this on local development):
JS error in the console after clicking on an element:

Uncaught TypeError: Cannot read property '__sentry_instrumentation_handlers__' of undefined"

This breaks event propagation and the element click handlers don't fire. We were not having this issue with the previous build (and may be unrelated to the OP issue)

[kamilogorek]

@clinejj see https://github.com/getsentry/sentry-javascript/issues/3221#issuecomment-771069696 (it was caused due to misused event listener method in the end-users code)

[r3c]

It was never possible. We have a strict version requirement in package.json file of every package we release.
We don't use carrets nor tildes there for a reason, and package resolution should correctly reflect that. If not, then it means that lock files are incorrectly generated and should be purged and rebuilt.

Got it. However this change was an API-breaking one, and I was under the impression that you were following semver, in which case bumping to 7.0.0 could have been an explicit way to highlight the change. Maybe I was wrong here and your versioning is following a different semantic?

[dubzzz]

@kamilogorek I perfectly understand that there is _strict version requirement_.

But actually it is not fully the case... Nothing prevents users from using @sentry/[email protected] in conjunction to @sentry/[email protected]. Maybe you should at least add some kind of peerDependencies between those to ensure your users will not pull them at incompatible versions.

As a user doing:

npm i --save-dev @sentry/[email protected]
npm i --save-dev @sentry/[email protected]

Does not raise any warnings.

[kamilogorek]

Maybe you should at least add some kind of peerDependencies between those to ensure your users will not pull them at incompatible versions.

Agree, although there's one problem with that based on my tests. When dependency is marked as peer, it will emit warning, and may break build in some configurations.
There's a way around that, by adding "peerDependenciesMeta": { "@sentry:browser": { "optional": true } } for example, but then, it doesn't enforce the correct version resolution in either yarn or npm. optionalDependencies are opt-out not opt-in, so they are not much better than regular dependencies field. What we want to mitigate, is installing @sentry/browser for node users, and @sentry/node for browser users, yet somehow enforce correct version resolution.