-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(remix): add esm export for node #12663
Conversation
Thanks for the PR @topaxi, I have tested this locally, which caused the server/client trace-propagation to break on E2E tests. I'll try to check what caused that. |
@topaxi, could you please check if the update here solves your issue? |
@onurtemizkan no, this will still run the package in CommonJS mode. |
What does work for me though, is to specify the default import. I have updated the PR :) Sadly I'm not able to install the npm dependencies locally from this repository as yarn does not succeed and playwright install tries to call |
Thanks for the update @topaxi. Yes, I can see that specifying |
This was specifically for Switching sentry/remix to ESM fixed it (I assume previously, sentry instrumented only the CJS versions of the package(s), which aren't actually run in the application). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -28,6 +28,9 @@ | |||
"import": "./build/esm/index.client.js", | |||
"require": "./build/cjs/index.client.js" | |||
}, | |||
"import": { | |||
"default": "./build/esm/index.server.js" | |||
}, | |||
"node": "./build/cjs/index.server.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this get split into the following?
"node": "./build/cjs/index.server.js" | |
"node": { | |
"import": "./build/esm/index.server.js", | |
"require": "./build/cjs/index.server.js" | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial change, see the first comment as to why that did not work.
Thanks for the fix @topaxi! |
This change broke our E2E tests so we'll likely revert. |
We are running remix in ESM mode, but the exports in package.json is pointing to the CommonJS module, which results in the SDK running in CommonJS mode instead of ESM.
This lead to the instrumentations not running/detecting properly.