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

Undocumented removal of default export in v8 #12013

Closed
3 tasks done
MattIPv4 opened this issue May 14, 2024 · 4 comments
Closed
3 tasks done

Undocumented removal of default export in v8 #12013

MattIPv4 opened this issue May 14, 2024 · 4 comments

Comments

@MattIPv4
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.0.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

N/A

Steps to Reproduce

npm init -y

[...]

npm i @sentry/node@7

[...]

node --input-type=module --eval "import Sentry from '@sentry/node'; console.log(Sentry.SDK_VERSION);"

7.114.0

npm i @sentry/node@8

[...]

node --input-type=module --eval "import Sentry from '@sentry/node'; console.log(Sentry.SDK_VERSION);"

file://[snip]/[eval1]:1
import Sentry from '@sentry/node'; console.log(Sentry.SDK_VERSION);
       ^^^^^^
SyntaxError: The requested module '@sentry/node' does not provide an export named 'default'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)
    at async ModuleLoader.eval (node:internal/modules/esm/loader:212:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

Node.js v20.12.2

Expected Result

Default export continues to exist, or the change is documented as a breaking change in the vb8 migration guide.

Actual Result

Not documented as far as I can tell, and no longer a default export.

@mydea
Copy link
Member

mydea commented May 14, 2024

Hey, thanks for writing in.

So we never supported this (it is also not documented anywhere), and if that ever worked it was accidental, not on purpose.
I think it may be related to us previously not actually having a proper ESM output, while we now do have a valid ESM output, so maybe it is becoming more strict because of this 🤔

If you look into any of our documentation, you'll see that you have to use import * as Sentry from '@sentry/node'. A default export has potential issues with tree shaking etc, which is why we recommend & document the namespace format instead. I don't even know how this used to work before, because if you build @sentry/node on the v7 branch, there is also no default export in the emitted build/esm/index.js file 🤔 Must be some weird interop thing!

We are unlikely to add a default export (and we never had one! See: https://github.com/getsentry/sentry-javascript/blob/v7/packages/node/src/index.ts), so you'll have to change to the supported syntax (either using a namespace import or importing the methods you want directly).

@MattIPv4
Copy link
Author

Disagree with this being closed and this change remaining undocumented. Whether it was documented as a valid way to use Sentry or not should not matter here... it worked before for all of v7 as far as I am aware (we have always imported it this way) and per Hyrum's Law, it will be something that folks as using (especially as it is the default way to import a package).

I would ask that you document this as a breaking change in the migration guide if there is no intention to re-add a default export.

@FozzieHi
Copy link

I agree with Matt here, although it wasn't documented before I think some people will have naturally used it as a default import if they just see a code snippet in the documentation without the import block in view, in fact some IDEs will do this automatically for you. What's worse, and is surprising to me is that this isn't caught by TypeScript, the code builds fine but then errors at runtime with the error above. Not knowing much about the inner workings of TypeScript, is that a types issue here or perhaps just not supported by TypeScript?

@mydea
Copy link
Member

mydea commented May 17, 2024

I agree with Matt here, although it wasn't documented before I think some people will have naturally used it as a default import if they just see a code snippet in the documentation without the import block in view, in fact some IDEs will do this automatically for you. What's worse, and is surprising to me is that this isn't caught by TypeScript, the code builds fine but then errors at runtime with the error above. Not knowing much about the inner workings of TypeScript, is that a types issue here or perhaps just not supported by TypeScript?

I think this comes from how TS handles certain things internally and does some compat thing. As we never really supported this, and I think depending on your TS config this never worked, but with some TS configs it seemed to kind of work for whatever reason.

I agree though that it makes sense to call this out in the migration docs, I'll add a note on this! 👍

See: #12100

mydea added a commit that referenced this issue May 17, 2024
…rk (#12100)

It was brought up
#12013 that
importing Sentry as a default import is not working anymore in v8. While
this was never officially supported (and also not "removed" on purpose,
it just seemed to have worked incidentally before), I do not really
consider this a "breaking change", but, as pointed out in the issue, it
is still helpful to point this out in the migration docs, for users who
incidentally did this before.

We should also add this to migration docs on docs.sentry.io.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants