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

ESM support #3277

Closed
Toxicable opened this issue Jan 24, 2022 · 5 comments
Closed

ESM support #3277

Toxicable opened this issue Jan 24, 2022 · 5 comments
Labels
Can Close? We will close this in 30 days if there is no further activity

Comments

@Toxicable
Copy link

Toxicable commented Jan 24, 2022

🚀 feature request

Relevant Rules

  • ts_project
  • nodejs_binary
  • jasmine_node_test
    Maybe others, but these are the ones I'm familiar with

Description

  • Add support for executing JS that is running in ESM format (nodejs_binary, jasmine_node_test)
  • Add support for compiling to ESM (ts_project) (if needed)

Describe alternatives you've considered

I don't believe there's alternate ways to run ESM JS without core support in rules_nodejs


The primary motivation for this feature is for support of Angular v13 which only publishes ESM JS.
The general rule with ESM is that an ESM file can import CJS or ESM, and CJS can require CJS and import ESM only via the import() function. Therefore you cannot require() ESM from a CJS file.
This means that a codebase which runs a jasmine_node_test as it stands is unable to import Angular v13 (or any other ESM only packages)

(please correct me if this is wrong)
My understanding of how NodeJS picks what module format to run a file in is determined by:
First it will look at file extention: .cjs -> CJS, .mjs -> ESM
If the extension is .js then it looks at the package.json field "type", where module -> ESM, commonjs -> CJS
See "ESM_FILE_FORMAT" in the NodeJS docs for a detailed algorithm

With the above in mind I think the changes required to support ESM with nodejs_binary would be to rename internal JS files (node_patches.js, jasmine_runner.js) to be *.cjs, with the exception of loader.js which will need to be modified to be able to import() the users entry_point

For ts_library I think the most unambiguous method would be to have it output .mjs files, however that's not currently supported and there's a lot of discussion, see some of the threads around here microsoft/TypeScript#18442
Therefore I think the next best option is just to output .js files then the user will need to ensure they have a package.json with type=module in order to declare the output to be ESM, this will need some docs.
That would mean no changes are needed to the rule itself.

NodeJS docs: https://nodejs.org/api/esm.html

@Toxicable
Copy link
Author

Toxicable commented Jan 25, 2022

I order to support jasmine_node_test the spec files need to be .mjs in order for the jasmine spec loader to be able to import it correctly. Ref https://github.com/jasmine/jasmine-npm/blob/main/lib/loader.js#L13
This could be done by adding a module attribute to the ts_project then calling a simple rule to rename outputs when module=esm (esm is just placeholder, values tbd). The attribute would be required when typescript adds support for .mjs outputs anyway since we'd need to pre-declare the .mjs outputs.
Alternatively we could suggest that the rename rule is declared and called from the users codebase to reduce the maintenance overhead of rules_nodejs

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Jul 28, 2022
@fabianlema
Copy link

Any progress on ESM support? Is required in Angular 13

@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Aug 1, 2022
@github-actions
Copy link

github-actions bot commented Feb 1, 2023

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Feb 1, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This issue was automatically closed because it went 30 days without any activity since it was labeled "Can Close?"

@github-actions github-actions bot closed this as completed Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity
Projects
None yet
Development

No branches or pull requests

2 participants