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

feat: Emit *.mjs files across all packages #10833

Closed
wants to merge 21 commits into from
Closed

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Feb 27, 2024

#10046

On the road of proper esm support, this first change introduces package.json exports to each of our packages. Now lets see what breaks 😭

This PR has some breaking changes or things that have been disabled to get the build working and tests passing which will need to be revisited later:

  • The modulesIntegration uses require to get the module list and makes no sense for ESM
  • Uses of require('inspector') or `dynamicRequire('inspector') have been replaced with regular imports. We can't use async import because it introduces async where it can't be supported. This change means we will break Vercel/pkg apps since they build Node without inspector. Vercel/pkg has been deprecated as of Jan 2024.
  • nativeNodeFetchIntegration is no longer in the default integrations and the export has been removed for now. This integration uses require and when I tried import('opentelemetry-instrumentation-fetch-node') it seemed seemed to break Rollup output in a similar way to this: preserveModules creates wrong output rollup/rollup#3743

Other outstanding issues:

  • Astro tests failing due to Error: Failed to resolve entry for package "https"

This comment was marked as outdated.

@timfish
Copy link
Collaborator

timfish commented Mar 1, 2024

Node/Remix Integration tests are now passing. Still a few more issues to work through...

@timfish
Copy link
Collaborator

timfish commented Mar 1, 2024

@lforst
Copy link
Member

lforst commented Mar 4, 2024

@timfish We likely have to update the paths here:

const apiWrapperTemplatePath = path.resolve(__dirname, '..', 'templates', 'apiWrapperTemplate.js');

@timfish

This comment was marked as outdated.

@timfish timfish marked this pull request as draft March 4, 2024 15:09
@lforst
Copy link
Member

lforst commented Mar 5, 2024

The nextjs e2e test failures are weird. Next.js should not see the "wrapping target file" paths at all. They should all be bundled away with the rollup call here:

@Lms24
Copy link
Member

Lms24 commented Mar 5, 2024

@Lms24 A quick search suggests you've seen and fixed this error before! (vitejs/vite#12439)

IIRC my project's node_modules back then actually contained a strange npm package that was used instead of the builtin https package. Don't recall why this package was installed in the first place though

@timfish
Copy link
Collaborator

timfish commented Mar 5, 2024

image

I've fixed this previous issue with nextjs which was caused by the build wrapping script being removed. This resulted in cjs templates being used rather than esm. The server tests are all now passing!

However, most of the client playwright tests are failing:

image

@timfish
Copy link
Collaborator

timfish commented Mar 5, 2024

The global issue ended up being caused by #10925

AbhiPrasad pushed a commit that referenced this pull request Mar 5, 2024
Found while working through ESM issues in #10833.

For whatever reason this passes all the integration tests until ESM is
used 🤯
@timfish
Copy link
Collaborator

timfish commented Mar 5, 2024

Closing in favour of #10928

@timfish timfish closed this Mar 5, 2024
AbhiPrasad pushed a commit that referenced this pull request Mar 7, 2024
Found while working through ESM issues in #10833.

For whatever reason this passes all the integration tests until ESM is
used 🤯
AbhiPrasad pushed a commit that referenced this pull request Mar 8, 2024
Some bundlers, especially older ones, don't support resolving `.mjs`
files out of the box. To retain compatibility with these bundlers
without requiring users to adjust their configurations, we need to keep
all our ESM output as `.js` files.

So node.js loads these as modules, we output a `package.json` into each
`./build/esm` output directory containing simply `{ "type": "module" }`.
This works because whenever node.js is asked to load a `.js` file, it
tries to work out whether this is CJS or ESM by searching up through
parent directories until it finds a `package.json` with the `type` set.

This PR:
- Adds a Rollup plugin that injects the `package.json` into the ESM
output
- Adds the package `exports` that @AbhiPrasad worked out for #10833
- Fixes an import issue with `next/router` which is CJS (at least in
v10)
- Fixes an import issue with `@prisma/instrumentation` which is CJS
- Ensures that CJS only integrations are not included in the
`@sentry/node` default integrations when running as ESM

This PR also makes some unrelated changes:
- Changes to the old Node SDKs to allow the tests to pass
- Removes the bundle size analysis CI job as it doesn't appears to be
compatible with the node ESM output
@AbhiPrasad AbhiPrasad deleted the abhi-esmify branch March 14, 2024 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants