Skip to content

fix: esm json import#416

Merged
bcoe merged 10 commits intoyargs:mainfrom
jly36963:fix-broken-esm-json-import
Nov 14, 2021
Merged

fix: esm json import#416
bcoe merged 10 commits intoyargs:mainfrom
jly36963:fix-broken-esm-json-import

Conversation

@jly36963
Copy link
Contributor

@jly36963 jly36963 commented Oct 10, 2021 •
edited
Loading

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.

@jly36963 jly36963 requested a review from bcoe October 10, 2021 20:48
@bcoe
Copy link
Member

bcoe commented Oct 13, 2021

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

@jly36963
Copy link
Contributor Author

jly36963 commented Oct 13, 2021

@bcoe sounds good

@bcoe
Copy link
Member

bcoe commented Oct 20, 2021

@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
Copy link
Contributor Author

jly36963 commented Oct 20, 2021 •
edited
Loading

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

bcoe
bcoe requested changes Oct 23, 2021
View reviewed changes
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/index.ts Outdated
return require(path)
} else if (path.match(/\.json$/)) {
return readFileSync(path, 'utf8')
return esmRequire(path)
Copy link
Member

@bcoe bcoe Oct 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jly36963 marked this pull request as draft October 24, 2021 21:48
@jly36963
Copy link
Contributor Author

jly36963 commented Oct 24, 2021

@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
Copy link
Member

bcoe commented Oct 27, 2021

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

@hybrist
Copy link
Contributor

hybrist commented Oct 28, 2021 •
edited
Loading

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
Copy link
Member

bcoe commented Nov 8, 2021

@jkrems thank you for the review.

@bcoe bcoe marked this pull request as ready for review November 14, 2021 23:54
bcoe
bcoe approved these changes Nov 14, 2021
View reviewed changes
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where this work landed 👍

@bcoe bcoe merged commit 90f970a into yargs:main Nov 14, 2021
@jly36963 jly36963 mentioned this pull request Mar 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bcoe bcoe bcoe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jly36963 @bcoe @hybrist @sTEVE7611