Issue 2105 more detailed error

[michael-lloyd-morris]

🤔 What's changed?

The error message viewed when importing a CommonJS module or requiring an ES module is now customized.

⚡️ What's your motivation?

Making life easier when simple mistakes are made. The error message Node gives when you try to import steps using CommonJS format or require an ES module are a bit cryptic. The new error message tells the user they are using the wrong directive and to either change the file's format or use the other directive to include the file.

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

📋 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.

[coveralls]

Coverage: 98.489% (-0.04%) from 98.525% when pulling c0fa54d on michael-lloyd-morris:issue-2105-more-detailed-error into aae711c on cucumber:main.

[davidjgoss]

One thing we need to account for here is that, unlike the other way around, it's perfectly valid to import() a CommonJS module.

The example in the feature file yields an error from Node.js because the file is evaluated as an ES module (due to its .mjs suffix) but contains require() calls so is effectively invalid JavaScript from the engine's perspective. If that file was .cjs or even .js (assuming no `type="module" on the package) it would work fine with the import option.

I think the issue can also show up where you use an ESM loader but it outputs code with require() in - again because the file purports to be a module but contains require() calls. This could be easily done if you misconfigure ts-node for example.

It's probably worth catching that error still in the importer, but I think the messaging will need to be more along the lines of "it looks like some of your support code is using require syntax where only imports are expected" and a maybe a link relevant Node.js docs.

[michael-lloyd-morris]

I did not know that it's valid to import a common JS module. That does explain why the error messages are so different. Of the two native nodejs errors that one is the more easy to understand, so I'll remove the catch on the importer for now to satisfy this requirement along with the scenario testing for that code change. Shall I keep the remaining feature alone in it's own file or move it into the esm.feature file? (on second thought, just going ahead and doing that as it makes the most sense, but I can undo it).

[michael-lloyd-morris]

Requested Changes applied.

[michael-lloyd-morris]

Is there something else I need to do or where the changes I did insufficient?

[davidjgoss]

Just a couple of things to address but this is looking like a good change. Also see a conflict on the changelog file.

[michael-lloyd-morris]

I'll get to these items in the morning and will try to be done by end of day, though I've got to take a chunk of the day to take my Dad to the hospital.

(Until I have a job, volunteer work here is my job - if only to keep my skills sharp and my sanity intact - so I'm treating it as such).

[michael-lloyd-morris]

Ok, all requested changes applied, though see the note about Typescript and using the cause option on Error throws.