Readme CJS example fails with versions of Node older than 12.17

[gpoole]

Using the CJS example code from the README.md seems to fail for me on all Node versions I tried before 12.17 (12.16.1, 12.15.0, 12.14.1 and 11.15.0):

#!/usr/bin/env node
const yargs = require('yargs/yargs')
const { hideBin } = require('yargs/helpers')
const argv = yargs(hideBin(process.argv)).argv

if (argv.ships > 3 && argv.distance < 53.5) {
  console.log('Plunder more riffiwobbles!')
} else {
  console.log('Retreat from the xupptumblers!')
}

The error varies by version.

12.16.1:

Error: Cannot find module 'yargs/helpers'

12.14.1 and 12.15.0:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: <snip>/yargs-node-error/node_modules/yargs/helpers.mjs

11.15.0:

Error: Cannot find module 'yargs/helpers'

Works fine in 12.17 and 12.18.

Here's a sample of how I set it up: https://github.com/gpoole/yargs-node-error

[apoorv-mishra]

I researched a bit and found that yargs/yargs also exports hideBin here, therefore following change fixes the problem.

#!/usr/bin/env node
const yargs = require('yargs/yargs')
const { hideBin } = yargs
const argv = yargs(hideBin(process.argv)).argv

if (argv.ships > 3 && argv.distance < 53.5) {
  console.log('Plunder more riffiwobbles!')
} else {
  console.log('Retreat from the xupptumblers!')
}

[bcoe]

@apoorv-mishra @gpoole I think we should update the docs wherever we use require, to use the approach @apoorv-mishra outlines.

If anyone feels like submitting a patch, we would happily take the contribution 😄

[ef4]

There will probably need to be a corresponding update to the types too, because they don't cover this alternative location for hideBin.

[ljharb]

Could we add a helpers.js instead (alongside helpers.mjs), so the same entry point works for everyone?

[bcoe]

@ljharb @ef4 I'd be open to this. Would it be enough to add the helpers.js file, or are there Node.js versions that would also need an update to our exports map?

[ljharb]

For pre-exports node, that file would be sufficient - for post-exports node, you may want to modify the “require” condition, but the entry point will at least work for CJS already.

[gpoole]

Not sure if this helps and I haven't been able to get to the bottom of it, but I had a go at adding helpers.js and had problems in 12.16. It felt a lot to me like like 12.16 wasn't using the exports map, or I couldn't get it to work. If I added helpers.js as a CJS module, 12.16 would try to import that when I used require('yargs/helpers') and then errored because it should be an ESM:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: <snip>/node_modules/yargs/helpers.js
require() of ES modules is not supported.
require() of <snip>/node_modules/yargs/helpers.js from <snip>/yargs-node-error/index.cjs is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename helpers.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from <snip>/node_modules/yargs/package.json.

Since there's an export map for helpers that has a require entry I'm confused why this doesn't seem be being picked up at all by 12.16. As far as I can tell, adding helpers.cjs doesn't help either as it's not automatically picked up by doing require('yargs/helpers').

[ljharb]

@gpoole 12.16 doesn’t look at “exports” at all, that’s why the file needs to be added where pre-exports node can find it.

[ljharb]

If the root is using type module (which I’d discourage), then you can do a helpers directory with an index.js file, and a package.json with type “commonjs”.

[gpoole]

Yeah, that does seem to be the case, but I was thrown because the Node documentation seemed to me to say conditional exports were introduced in 12.16 (and 13.2.0?). Are the docs wrong there or am I misunderstanding it? Seems like actually 12.17 was the version it was added?

[ljharb]

Added in 12.16 behind a flag; the flag was removed in 12.17.

[bcoe]

@gpoole thank you for the bug report, this should be patched in the latest release now. @ljharb thanks for the idea of nesting inside helpers/ with an additional package.json; It feels like the latest release of yargs solves this issue, and cleans up the directory structure a bit.

[gpoole]

Thanks @bcoe! Works perfectly now.