-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
cli: esm support #1589
cli: esm support #1589
Conversation
5332973
to
c8943bc
Compare
f072a14
to
01de7f2
Compare
@charlierudolph some work to do still but would be good to get your take on this. We're dealing with this on two fronts:
|
Cool. I think approach makes sense to me. To check my understanding we need a second binary as we need node to load things differently? I would prefer if we could have a |
Done - was able to make this work by keeping an The last part was Windows, where trying to |
|
||
To enable this, run with the `--esm` flag. | ||
|
||
This will also expand the default glob for support files to include the `.mjs` file extension. |
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.
Is ESM purely additive (ie require is still supported)?
If not, would seem like using this requires custom formatters used would also need to be written in ESM
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.
ESM is additive. If enabled, it'll use import()
to load all user code (steps/hooks, formatters, snippets, and config file). If any of this user code is CommonJS and has require()
s in it, that will still work fine - you can import()
CommonJS modules, but you can't require()
ES modules.
Eventually (after dropping Node 12 support, I think) we'll be able to just always use import()
and not need the flag.
(Hope I've understood the question right!)
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.
And I've pushed some documentation improvements to this effect.
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.
Looks good. Thanks!
importers.js
Outdated
@@ -0,0 +1,17 @@ | |||
/* |
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.
Thoughts on putting this in src and as part of building copy it into lib? Same for wrappers.mjs.
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.
Good point - done!
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.
Just the last comment about the location of a couple files. Otherwise looks great! Thanks for doing this!
This reverts commit 1a8bc79.
Motivation
#1304
Support ESM usage with minimal additional flags/configuration and without dropping support for Node 10.
Details
To allow importing cucumber functions from ESM support code, provide a "wrapper" mjs entry point for ESM projects as described in https://nodejs.org/api/packages.html#packages_approach_1_use_an_es_module_wrapper.
Usage:
The
--esm
flag causes us to useimport()
instead ofrequire()
for loading support code - the former works for both ESM and commonjs, where the latter only works for commonjs. When we drop Node 10 support, we'll be able to deprecate/remove the flag and just useimport()
across the board.I think we need to consider usage of this in conjunction with on-the-fly transpilers as out of scope for now. For example ts-node is at a very experimental phase with ESM per TypeStrong/ts-node#1007.
Test project: https://github.com/davidjgoss/cucumber-esm-example
Todo
import()
orrequire()
appropriately at runtime for support code.mjs
files by default if using ESM