sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

SDK fails in ESM mode in combination with date-fns

Open Xhale1 opened this issue 1 year ago • 10 comments

Is there an existing issue for this?

  • [X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
  • [X] I have reviewed the documentation https://docs.sentry.io/
  • [X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

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.

Xhale1 avatar May 22 '24 03:05 Xhale1

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

timfish avatar May 22 '24 10:05 timfish

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.

dankochetov avatar May 22 '24 11:05 dankochetov

adding this line fixes it for me.

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

timfish avatar May 22 '24 15:05 timfish

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!

Xhale1 avatar May 22 '24 17:05 Xhale1

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'?

timfish avatar May 22 '24 22:05 timfish

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: https://github.com/date-fns/date-fns/pull/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.~~

Xhale1 avatar May 22 '24 23:05 Xhale1

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";

Xhale1 avatar May 22 '24 23:05 Xhale1

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

lhermann avatar May 24 '24 09:05 lhermann

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.

timfish avatar May 24 '24 10:05 timfish

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

lhermann avatar May 24 '24 10:05 lhermann

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!

mydea avatar Jun 07 '24 08:06 mydea

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.

Xhale1 avatar Jun 07 '24 16:06 Xhale1