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

use import wherever possible to load code #2334

Merged
merged 13 commits into from Oct 8, 2023
Merged

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented Oct 4, 2023

πŸ€” What's changed?

Previously our means of supporting ESM configuration files, formatters and snippet syntaxes was just to attempt a require() and then fall back to await import() in a catch block if it failed. This was a temporary measure, which this PR replaces with something more permanent.

Due the different structures of ESM vs CommonJS when imported, this has a little complexity. For formatters and snippet syntaxes, we can switch to await import() and just walk a couple of levels down the object looking for a default key that is a function.

For config, the logic above would be too fuzzy (profile keys of "default" etc), so we use await import() or require() as appropriate based on the file extension. For .js files we check the nearest package.json for the type and take that hint on whether to use require() or await import(). Also, throws if the extension isn't one of our supported ones, and adds some debug logging.

Also, we've been able to remove the hacky importer.js mechanism, which was added to get around TypeScript's behaviour (at the time) of transpiling all await import() statements down to Promise-wrapped require()s - the module mode node16 now solves this.

This will be a breaking change for any users who were relying on our internal usage of require() to dynamically transpile configuration files, formatters or snippet syntaxes (i.e. by registering a transpiler first). The suggested change will be to transpile beforehand and load the output file.

⚑️ What's your motivation?

#2059:

Configuration files are loaded with the appropriate mechanism based on file/package type
Custom formatters/snippets are only loaded with await import()

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)
  • πŸ’₯ Breaking change (incompatible changes to the API)

πŸ“‹ 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.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@davidjgoss davidjgoss added the πŸ’” breaking change This will require a major release label Oct 4, 2023
@davidjgoss davidjgoss added this to the 10.0.0 milestone Oct 4, 2023
@davidjgoss davidjgoss changed the title import or require config based on extension and/or package use import wherever possible to load code Oct 8, 2023
@coveralls
Copy link

Coverage Status

coverage: 97.872% (-0.08%) from 97.952% when pulling d58e575 on feat/always-import-config into eb1890f on main.

@davidjgoss davidjgoss marked this pull request as ready for review October 8, 2023 15:40
@davidjgoss davidjgoss merged commit 3d23eec into main Oct 8, 2023
6 checks passed
@davidjgoss davidjgoss deleted the feat/always-import-config branch October 8, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
πŸ’” breaking change This will require a major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants