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

SDK fails in ESM mode in combination with date-fns #12154

Closed
3 tasks done
Xhale1 opened this issue May 22, 2024 · 12 comments · Fixed by #12388
Closed
3 tasks done

SDK fails in ESM mode in combination with date-fns #12154

Xhale1 opened this issue May 22, 2024 · 12 comments · Fixed by #12388
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@Xhale1
Copy link

Xhale1 commented May 22, 2024

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.2.1

Framework Version

Nodejs 20.13.1

Link to Sentry event

https://mycopilot.sentry.io/issues/5344731586/?project=6067364&query=is%3Aunresolved&referrer=issue-stream&statsPeriod=24h&stream_index=0

SDK Setup

Sentry.init({
  dsn: "[REDACTED]",
});

Steps to Reproduce

  1. Upgrade from v7 to v8 following the migration docs
  2. Follow Sentry docs for ESM
  3. Add --import ./dist/instrument.js to the node command

Expected Result

Application starts up.

Actual Result

Application crashes with an error which seems to involve duplicate exports with ESM. An example of my particular error:

SyntaxError: Identifier '$longFormatters' has already been declared
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:169:18)
    at callTranslator (node:internal/modules/esm/loader:272:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:278:30)

Upon investigating, I found my error to be related to the date-fns package and this issue. Reverting date-fns to a version without a duplicate export results in another similar error (I suspect related to the openai package duplicate export though haven't confirmed).

This issue does not occur when using the --import flag to load a file without sentry code in it, and it doesn't surface elsewhere. This error may be due to other projects misusing ESM, though I hope since the issue only surfaces with Sentry instrumentation that it may be solvable here.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 May 22, 2024
@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label May 22, 2024
@timfish
Copy link
Collaborator

timfish commented May 22, 2024

Interesting, I wonder why the loader triggers this 🤔

Do you just have only the Sentry.init in ./dist/instrument.js and instrument.js fully ESM?

If you add this before Sentry.init, do you still get the error?

globalThis._sentryEsmLoaderHookRegistered = true

@dankochetov
Copy link

If you add this before Sentry.init, do you still get the error?

I'm having the same issue (just with a different import name) and adding this line fixes it for me.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 May 22, 2024
@timfish
Copy link
Collaborator

timfish commented May 22, 2024

adding this line fixes it for me.

Be aware that globalThis._sentryEsmLoaderHookRegistered = true disables OpenTelemetry auto-instrumentation so you will lose those features.

@Xhale1
Copy link
Author

Xhale1 commented May 22, 2024

globalThis._sentryEsmLoaderHookRegistered = true does allow my app to start properly, though as you mentioned without auto-instrumentation. Let me know if you need anything else!

ps. Upgrading our web frontends was a breeze, amazing work on v8!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 May 22, 2024
@timfish timfish self-assigned this May 22, 2024
@timfish
Copy link
Collaborator

timfish commented May 22, 2024

It looks like this is caused by import-in-the-middle. It uses a Map so when it encounters duplicate exports it should just result in one of them being exposed but for some reason it's not working. I'll dig into the code and see if I can work it out.

Do you know what date-fns import is causing this? I guess it's something like import { parse, format } from 'date-fns'?

@Xhale1
Copy link
Author

Xhale1 commented May 22, 2024

Edit: My apologies, I just realized you asked about my import, not date-fns's exports. I'll make a minimally reproducible repo, one moment.

I haven't verified myself but I the issue I linked has a PR which seems to suggest removing a longFormatters export will do the trick: date-fns/date-fns#3770

Conversation in that PR suggests that parse might additionally be responsible. I don't have a better grasp beyond that but I'm happy to do more digging.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 May 22, 2024
@Xhale1
Copy link
Author

Xhale1 commented May 22, 2024

I'm observing identical errors when adding import { parse } from "date-fns"; and when adding import { format } from "date-fns";. When neither import is included then our project builds successfully. I only need to import one of of them to observe the error.

The error also occurs for import * as datefns from "date-fns";

@mydea mydea changed the title SyntaxError: Identifier x has already been declared SDK fails in ESM mode in combination with date-fns May 23, 2024
@lhermann
Copy link

I'm running into the exact same error with date-fns v3. Disabling the instrumentation with globalThis._sentryEsmLoaderHookRegistered = true works for now.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 May 24, 2024
@timfish
Copy link
Collaborator

timfish commented May 24, 2024

There are outstanding PRs for import-in-the-middle that hopefully fix a wide range of issues.

There is a patch available here that can be used with patch-package that should fix all outstanding issues.

@lhermann
Copy link

@timfish After sheepishly having to ask ChatGPT how to apply this patch, I can confirm, it works 🎉 👍 .

@mydea
Copy link
Member

mydea commented Jun 7, 2024

Hello, we've just released v8.8.0, which should hopefully resolve this ESM problem. Let us know if you updated and are still running into problems!

@Xhale1
Copy link
Author

Xhale1 commented Jun 7, 2024

I can confirm that this issue is now resolved! Thank you for taking so much time to work on and coordinate this fix across sentry, OTEL, and import-in-the-middle. I appreciate it!

Unfortunately we're now running into an issue with the openai package and ESM instrumentation (this appears to be the final blocker for our codebase to adopt v8 + ESM). I'll create a new issue.

billyvg pushed a commit that referenced this issue Jun 10, 2024
resolves #12242
(although there are still some follow ups)

https://github.com/open-telemetry/opentelemetry-js/releases/tag/v1.25.0

I think this lockfile looks correct, but lmk if this feels off.

resolves #12011
resolves #12059
resolves #12154
resolves #12237
resolves nodejs/import-in-the-middle#77 cc
@mohd-akram
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants