Closed jly36963 closed 2 years ago
@jly36963 let's make a separate PR that drops Node 10 in yargs-parser
?
@bcoe sounds good
@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.
@bcoe Let me know what else I can do on https://github.com/yargs/yargs-parser/pull/421
@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?
@jkrems can you help figure this out, I want to make sure we don't break Google's use case.
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.
@jkrems thank you for the review.
Addresses: https://github.com/yargs/yargs/issues/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 cjsrequire
, there are two easy solutions.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, ascreateRequire
was implemented in node 12. If that's a deal-breaker, I'm cool with switching to theJSON.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.