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

build(package): ensure NodeJS considers ESM .js files as ESM #7130

Closed
wants to merge 2 commits into from

Conversation

devversion
Copy link

@devversion devversion commented Dec 9, 2022

Concrete issue

When you import rxjs from NodeJS from an ESM file, the loading
of the RxJS files fails with syntax errors because NodeJS treats
them as CommonJS.

Background:

NodeJS treats JavaScript files as ESM when one of the following is true:

  • The file is explicitly named .mjs
  • The file is part of a directory where the closest package.json has
    type: module.

Both things are not applying for the ESM output in dist/esm and
dist/esm5 so these ESM artifacts cannot be used directly in NodeJS
because NodeJS will attempt loading them as CommonJS.

Note that this was not noticeable with e.g. bundlers like Webpack
as those do not rely on the NodeJS semantics for "detecting" ESM.

Related issue (if exists):

#6321

Fixes that there was logic still applying to the non-existent
`esm2015` folder. This commit changes it to `esm` which seems
to be the target-independent main ESM output.
@devversion devversion marked this pull request as ready for review December 9, 2022 16:58
NodeJS treats JavaScript files as ESM when one of the following is true:

 * The file is explicitly named `.mjs`
 * The file is part of a directory where the closest `package.json` has
   `type: module`.

Both things are not applying for the ESM output in `dist/esm` and
`dist/esm5` so these ESM artifacts cannot be used directly in NodeJS
because NodeJS will attempt loading them as CommonJS.

Note that this was not noticeable with e.g. bundlers like Webpack
as those do not rely on the NodeJS semantics for "detecting" ESM.
@benlesh benlesh added AGENDA ITEM Flagged for discussion at core team meetings 8.x Issues and PRs for version 8.x labels Mar 7, 2023
@benlesh
Copy link
Member

benlesh commented Mar 7, 2023

Hey, @devversion, sorry for the slow response here. Anyone from the Angular team could have reached out to me directly to get my eyes on this sooner.

The problem I see is that this looks like a breaking change. So we'd be unable to release it until 8.x. Am I incorrect about that?

@benlesh benlesh self-assigned this Mar 7, 2023
@benlesh
Copy link
Member

benlesh commented Mar 7, 2023

I think another problem is that we're not really outputting "real" ESM, like I don't think we have file extensions in the output, etc. So this would probably break in newer versions of NodeJS anyway?

@devversion
Copy link
Author

Thanks @benlesh! Good question with the breaking change. I think you're actually right that this would break the ecosystem as Webpack for example specifically works with this "invalid ESM", ignoring missing extensions just because there is no explicit .mjs extension, or {type: module} nearby. https://webpack.js.org/configuration/module/#resolvefullyspecified

So yeah, this seems only useful to v8. I saw you there is other planned ESM work for v8, so feel free to close this if it would rather interfere then.

@devversion
Copy link
Author

For NodeJS use specifically, I think the output is already broken and this would improve the situation, but still not solve it completely- due to the missing extensions. In practice, no NodeJS user is ever getting the ESM output of rxjs at all because the node condition always points to the CJS output (and this is non-configurable)

@devversion
Copy link
Author

I'm closing this as I don't know what the next steps are, but I'm happy to re-open!

@devversion devversion closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants