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(node): allow executing esm compiled scripts #10414

Merged

Conversation

bulldog98
Copy link
Contributor

@bulldog98 bulldog98 commented May 21, 2022

Current Behavior

The @nrwl/node:node executor is not able to execute esm module scripts

Expected Behavior

The @nrwl/node:node executor is able to execute esm module scripts

Fixes: #10565
Fixes: #16036

@nx-cloud
Copy link

nx-cloud bot commented May 21, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit e6fdb7f.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@vercel
Copy link

vercel bot commented May 21, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) May 4, 2023 9:45pm

@bulldog98
Copy link
Contributor Author

Why did the build not run?

@nartc
Copy link
Contributor

nartc commented Jun 4, 2022

@bulldog98 , thank you for the PR. Can you add some e2e tests for this change?

@bulldog98
Copy link
Contributor Author

bulldog98 commented Jun 4, 2022

@bulldog98 , thank you for the PR. Can you add some e2e tests for this change?

Sure I first did not see there where e2e tests, but found them now.

@bulldog98
Copy link
Contributor Author

@nartc I was unable to run the e2e test locally, how are they executed?

@bulldog98 bulldog98 force-pushed the fix-node-executor-to-work-with-esm-modules branch from aeb56d6 to cb9ccb1 Compare June 4, 2022 08:21
@nartc nartc force-pushed the fix-node-executor-to-work-with-esm-modules branch from cb9ccb1 to cc0515b Compare June 9, 2022 19:53
@nartc
Copy link
Contributor

nartc commented Jun 9, 2022

@bulldog98 I've rebased the branch and kicked off the build which is failing. Can you take a look? To run test locally, you'd need to start local-registry with:

yarn local-registry enable
yarn local-registry start

then run the e2e:

yarn nx e2e e2e-node

@bulldog98
Copy link
Contributor Author

sure I'm working on fixing the test.

@bulldog98 bulldog98 force-pushed the fix-node-executor-to-work-with-esm-modules branch 2 times, most recently from 64c40ce to 215ea0e Compare June 11, 2022 11:46
@bulldog98
Copy link
Contributor Author

@nartc I fixed the e2e test it works now

@StringKe
Copy link

I had the same problem.
How can I use this pr to test if it's the same problem?

@nartc nartc force-pushed the fix-node-executor-to-work-with-esm-modules branch from 215ea0e to 902c406 Compare June 17, 2022 19:35
@bulldog98 bulldog98 force-pushed the fix-node-executor-to-work-with-esm-modules branch from 902c406 to 4aee47a Compare June 20, 2022 19:49
@bulldog98
Copy link
Contributor Author

@nartc sorry I had messed up the check to beEqual, but it should be a toContains

@nartc nartc force-pushed the fix-node-executor-to-work-with-esm-modules branch from 4aee47a to e6fdb7f Compare June 20, 2022 19:57
@nartc
Copy link
Contributor

nartc commented Jun 21, 2022

@bulldog98 Thanks for the PR. Everything looks good so far. Just one quick question, why did you have the following?

...
const dynamicImport = new Function('specifier', 'return import(specifier)');
...

Would this work as well?

...
function dynamicImport(specifier) {
   return import(specifier);
}

dynamicImport(fileToRun);
...

@luxaritas
Copy link
Contributor

That said, I'm curious why not just import(fileToRun)

@bulldog98
Copy link
Contributor Author

The problem is that typescript transpiles the import(fileToRun) to require(fileToRun) the same happens with the version of @nartc.

@luxaritas
Copy link
Contributor

The "real" fix would be to change "module" to "es2020" in the tsconfig - that would be a much larger change, but also this implicitly breaks environments that don't support the dynamic import syntax natively instead of explicitly. I think all current supported node versions should though?

@nartc
Copy link
Contributor

nartc commented Jun 21, 2022

@luxaritas yep, that's my concern as well. I also think the real fix is to support ESM across the board. With TS 4.7, we have a more "official" ESM support but we're somewhat waiting for the dust to settle before making substantial changes to all plugins.

@nrwl nrwl unlocked this conversation May 4, 2023
@AgentEnder AgentEnder reopened this May 4, 2023
@AgentEnder AgentEnder force-pushed the fix-node-executor-to-work-with-esm-modules branch from e6fdb7f to a1bb4a2 Compare May 4, 2023 18:02
@jaysoo jaysoo force-pushed the fix-node-executor-to-work-with-esm-modules branch 2 times, most recently from 4bc0aff to 1abc5d7 Compare May 4, 2023 19:01
@jaysoo
Copy link
Member

jaysoo commented May 4, 2023

We'll merge this fix, and introduce even more functionality to make sure CJS/ESM compatibility is consistent between the different technologies (webpack, esbuild, tsc, swc, etc.). This is a good workaround until then.

@veimox
Copy link
Contributor

veimox commented May 4, 2023

@jaysoo jaysoo force-pushed the fix-node-executor-to-work-with-esm-modules branch from 1abc5d7 to 3d85359 Compare May 4, 2023 20:03
@jaysoo jaysoo enabled auto-merge (squash) May 4, 2023 20:03
@jaysoo jaysoo force-pushed the fix-node-executor-to-work-with-esm-modules branch from 3d85359 to 64befff Compare May 4, 2023 20:43
lerna.json Outdated Show resolved Hide resolved
@jaysoo jaysoo force-pushed the fix-node-executor-to-work-with-esm-modules branch 2 times, most recently from 096f1b9 to 94196a6 Compare May 4, 2023 21:11
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Dynamic require of "*" is not supported Unable to execute node ES Modules