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

cli: esm support #1589

Merged
merged 34 commits into from Apr 17, 2021
Merged

cli: esm support #1589

merged 34 commits into from Apr 17, 2021

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented Feb 26, 2021

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:

cucumber-js --esm

The --esm flag causes us to use import() instead of require() 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 use import() 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

  • Use import() or require() appropriately at runtime for support code
  • Handle importing cucumber.js configuration file
  • Handle importing custom formatters
  • Handle importing snippets
  • Handle parallel runtime
  • Include .mjs files by default if using ESM
  • Test usage with ESM in CI
  • Resolve Windows issues
  • Documentation

@coveralls
Copy link

coveralls commented Feb 26, 2021

Coverage Status

Coverage decreased (-0.08%) to 98.494% when pulling 38febb5 on esm-maybe into 1109951 on master.

@davidjgoss
Copy link
Contributor Author

davidjgoss commented Feb 28, 2021

@charlierudolph some work to do still but would be good to get your take on this.

We're dealing with this on two fronts:

  • An alternative executable cucumber-es runs cucumber with an override that loads support code via dynamic imports
  • A wrapper entry point for ESM so that support code can natively import cucumber functions

@charlierudolph
Copy link
Member

charlierudolph commented Mar 7, 2021

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 --esm flag on our main binary that then just calls to second binary so our users don't need to know about the second binary (thought not certain if that runs into issues)

bin/cucumber-es.mjs Outdated Show resolved Hide resolved
@davidjgoss
Copy link
Contributor Author

davidjgoss commented Apr 7, 2021

@charlierudolph

I would prefer if we could have a --esm flag on our main binary

Done - was able to make this work by keeping an importer abstraction off the TypeScript compilation path, works nicely.

The last part was Windows, where trying to await import() absolute file paths has issues - see nodejs/node#31710, followed the pattern there using pathToFileURL where we'll be dealing with an absolute file path.

@davidjgoss davidjgoss marked this pull request as ready for review April 7, 2021 23:28
@davidjgoss davidjgoss changed the title esm support cli: esm support Apr 8, 2021
bin/cucumber-js Outdated Show resolved Hide resolved

To enable this, run with the `--esm` flag.

This will also expand the default glob for support files to include the `.mjs` file extension.
Copy link
Member

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

Copy link
Contributor Author

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!)

Copy link
Contributor Author

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.

Copy link
Member

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 @@
/*
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - done!

Copy link
Member

@charlierudolph charlierudolph left a 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!

@davidjgoss davidjgoss merged commit 1a8bc79 into master Apr 17, 2021
@davidjgoss davidjgoss deleted the esm-maybe branch April 17, 2021 10:23
davidjgoss added a commit that referenced this pull request Apr 21, 2021
davidjgoss added a commit that referenced this pull request Apr 21, 2021
* Revert "cli: esm support (#1589)"

This reverts commit 1a8bc79.

* update changelog
@davidjgoss davidjgoss mentioned this pull request Apr 22, 2021
2 tasks
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

3 participants