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: some deep requires don't work #1902

Closed
davidjgoss opened this issue Jan 29, 2022 · 5 comments
Closed

esm: some deep requires don't work #1902

davidjgoss opened this issue Jan 29, 2022 · 5 comments
Assignees
Labels
🐛 bug Defect / Bug
Milestone

Comments

@davidjgoss
Copy link
Contributor

Describe the bug

Not exactly about ESM, but a symptom of the way we've made the package "hybrid".

Some require statements that work fine when everything's CommonJS don't work with new ESM-aware Node versions and our module config.

To Reproduce
Steps to reproduce the behavior:

  1. Use @cucumber/cucumber@8.0.0-rc.2 and @cucumber/pretty-formatter
  2. Do a test run with --format @cucumber/pretty-formatter

Expected behavior

Normal test run with output from the formatter.

Actual behaviour

Error: Cannot find module '/Users/davidgoss/Projects/my-project/node_modules/@cucumber/cucumber/lib/formatter/helpers.js'

Additional context

The stack trace leads to this require statement in the pretty formatter's compiled code:

const helpers_1 = require("@cucumber/cucumber/lib/formatter/helpers");

We have this mapping in our package.json:

{
  "exports": {
    ".": {
      "import": "./lib/wrapper.mjs",
      "require": "./lib/index.js"
    },
    "./lib/*": {
      "require": "./lib/*.js"
    },
    "./package.json": "./package.json"
  },
}

The actual file it should be importing is:

node_modules/@cucumber/cucumber/lib/formatter/helpers/index.js

We'll need to either improve this exports mapping or rethink how we're accomodating ESM.

@davidjgoss
Copy link
Contributor Author

davidjgoss commented Jan 31, 2022

I've spent some time experimenting with this (based on extending an existing test d0a63b2), and I don't think we can do any better than what's there now unfortunately.

So I think the least worst strategy is:

  • Leave the exports config as it is
  • Document in the release notes that many deep import/requires will likely break
  • Fix forward by adding stuff to the main entry point that people need

@mattwynne
Copy link
Member

Sounds good to me. I always thought deep requires were a bit of a hack anyway, no?

@davidjgoss
Copy link
Contributor Author

@mattwynne agreed - they're always a symptom of something else.

@davidjgoss
Copy link
Contributor Author

davidjgoss commented Feb 17, 2022

Closing the loop on this, I went back over this comment from @jan-molak regarding deep import usages in the wild:

So in the next RC we should be all good.

@jan-molak
Copy link
Member

Thanks @davidjgoss - I'll give that a try soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug
Projects
None yet
Development

No branches or pull requests

3 participants