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: use specific option for import vs require for support code #1931

Merged
merged 17 commits into from Feb 26, 2022

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented Feb 23, 2022

🤔 Motivation and changes

As part of the initial work to support ESM, I made it so that all support code was imported by default, except sometimes --- based on heuristics around file extension and whether --require-module had been specified --- it fell back to require. It was a kind of aggressive move towards the new module system.

On reflection I think this was a mistake. The heuristics are bound to be wrong some of the time and potentially leave users with a breaking change that's difficult to work around. A future major release where we go ESM-only is probably not far away and so it makes sense to drop require entirely at that point.

Fortunately the ESM support has only gone as far as the 8.x RC so it's not too late to pivot. This PR makes it so that:

  • --require does the same as it always did before, no breaking changes
  • --import, a new option, works like --require but imports instead, so we'd expect ESM projects taking advantage of the new support in 8.x to use this
  • If neither --require or --import are specified, the automatic requireing does what it did before based on feature paths, but also any .mjs files found in the same way are imported - this is a backwards-compatible change because .mjs files didn't work at all previously

Not rowing back on formatters and snippets using import - I think this still right because it's much more predictable and there's no just-in-time transpilation involved to complicate things.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt (improvement to code design or tooling without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@davidjgoss
Copy link
Contributor Author

Still need to add and update some docs but feedback on the what and why would be great.

@coveralls
Copy link

coveralls commented Feb 23, 2022

Coverage Status

Coverage increased (+0.05%) to 98.21% when pulling a789e3b on import-own-config-option into 0195962 on main.

@davidjgoss davidjgoss changed the title Import own config option esm: use specific option for import vs require Feb 23, 2022
@davidjgoss davidjgoss changed the title esm: use specific option for import vs require esm: use specific option for import vs require for support code Feb 23, 2022
Copy link
Contributor

@aurelien-reeves aurelien-reeves left a comment

Choose a reason for hiding this comment

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

Looks good

src/run/paths.ts Show resolved Hide resolved
@davidjgoss
Copy link
Contributor Author

No objections so far; I'll update the documentation tonight and merge tomorrow.

docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Show resolved Hide resolved
docs/esm.md Show resolved Hide resolved
src/run/paths.ts Outdated Show resolved Hide resolved
@davidjgoss davidjgoss merged commit 416d3fc into main Feb 26, 2022
@davidjgoss davidjgoss deleted the import-own-config-option branch February 26, 2022 17:32
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