fix: esm json import

[jly36963]

Addresses: yargs/yargs#2040

Problem

The esm require for json files wasn't working properly. I offered a workaround in a comment on the issue above (passing a custom ConfigCallback to yargs.config()), but the underlying issue was the way yargs-parser handles the importing of the json file.

Solution

readFileSync(path, 'utf8') reads the json file as a string, but it needs to be a parsed object. To make it behave the same way as the cjs require, there are two easy solutions.

// use module createRequire
import { createRequire } from "module";
const esmRequire = createRequire(import.meta.url);

const config = esmRequire(path);

// parse the json string
const config = JSON.parse(readFileSync(path, 'utf8'));

I chose the createRequire solution, which required me to change the modules from es2015 to es2020 in tsconfig. I believe it would also require dropping support for Node 10, as createRequire was implemented in node 12. If that's a deal-breaker, I'm cool with switching to the JSON.parse solution instead.

Note

It looks like optimize and typescript are at odds now. Because of the TS 4.4 breaking change, I need to add types to the catch block errors, but optimize doesn't like it.

[bcoe]

@jly36963 let's make a separate PR that drops Node 10 in yargs-parser?

[jly36963]

@bcoe sounds good

[bcoe]

@jly36963 a little buy this week, but have this on my mind.

If you don't get to it, I'll work on dropping Node 10 later in the week so we can merge.

[jly36963]

@bcoe Let me know what else I can do on #421

[bcoe]

Left a note about adding a couple tests.

I believe the failing tests are due to a regression with @types/node, upgrading the dependency should help.

[bcoe]

It would be good to add one test that exercises this line.

Perhaps we can add a yargs-parser.mjs test to exercise this path? I believe it might also be good to add a test in yargs-parser.cjs, to confirm that we haven't broken JSON importing for CommonJS.

[jly36963]

@bcoe I'm not very familiar with tscc / tsickle / closure compiler. optimize is failing because tsickle overrides module flag as commonjs, which breaks import.meta. Do you know how to fix this?

failed run

[bcoe]

@jkrems can you help figure this out, I want to make sure we don't break Google's use case.

[jkrems]

This may break in general in environments without native import.meta which unfortunately includes code targeting CJS and also the closure compiler toolchain. We use a different index.ts inside of Google (because some of the dynamic expression in the current one already prevent optimization), so it will not actually break us in this case though.

Anyhow, drive-by comment on the PR: I'm not sure if you want the require to be relative to import.meta.url because that's relative to the yaqs-parser file somewhere in node_modules or relative to whatever ESM bundle it is in (which may not be the same as the bundle defining the config path). Since this is node only, maybe the correct value is actually createRequire('file:///') ("the file system root") or createRequire(pathToURL(join(process.cwd(), "_yargs-parser")))?

P.S.: If you're using createRequire (which I think is great and better than using the contextual require of the file!), maybe you could even do it consistently / independent of ESM or not? The non-import.meta.url variant I mentioned above would work equally well in ESM and CJS and could replace the current implementation in general.

[bcoe]

@jkrems thank you for the review.

[bcoe]

I like where this work landed 👍