yargs / yargs-parser

:muscle: the mighty option parser used by yargs
http://yargs.js.org/
ISC License
491 stars 120 forks source link

fix(deno): specify deps (avoids Deno warnings) #464

Closed rivy closed 9 months ago

rivy commented 1 year ago

The current version causes Deno to report warnings when imported. This PR pins the version to the current Deno std (0.159.0) and removes the warnings.

$ deno run -r deno.ts
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/path/mod.ts

$ git checkout fix.deno
Previous HEAD position was 3aba24c chore(main): release yargs-parser 21.1.1 (#455)
Switched to a new branch 'fix.deno'
Branch 'fix.deno' set up to track remote branch 'fix.deno' from 'origin'.

$ deno run -r deno.ts
rivy commented 1 year ago

@bcoe, review?

bcoe commented 1 year ago

Change looks good to me, but we seem to have a regression with the rollup dependency :/

rivy commented 1 year ago

I added a commit updating rollup and associated modules. It includes a necessary change from rollup-plugin-ts to @rollup/plugin-typescript.

It builds/tests successfully for me, locally and the CI workflow, with NodeJS v12+.

rivy commented 1 year ago

I revised the build changes to a minimal set that works with NodeJS v12+ and mirrored the changes to the yargs/y18n PR. There are some notsup complaints, specifically for NodeJS v12, but everything builds and tests correctly. If your CI passes then this should be complete.

Note, though these PR changes shouldn't have changed any coverage metrics, I am getting a coverage threshold error...

------------------------|---------|----------|---------|---------|-------------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s       
------------------------|---------|----------|---------|---------|-------------------------
All files               |   99.42 |    97.82 |     100 |   99.42 |                         
 index.ts               |    88.7 |    61.53 |     100 |    88.7 | 43-49                   
 string-utils.ts        |     100 |      100 |     100 |     100 |                         
 tokenize-arg-string.ts |     100 |      100 |     100 |     100 |                         
 yargs-parser-types.ts  |     100 |      100 |     100 |     100 |                         
 yargs-parser.ts        |     100 |    98.64 |     100 |     100 | 159,633,727-729,744,953 
------------------------|---------|----------|---------|---------|-------------------------
ERROR: Coverage for lines (99.42%) does not meet global threshold (99.5%)
ERROR: Coverage for statements (99.42%) does not meet global threshold (99.5%)

... if you want to add a test.

rivy commented 1 year ago

@bcoe , as the changes here in the PR didn't actually change any of the coverage tested files, how about just changing the thresholds for line and statement %'s to "99"?

Or, maybe better, use /* c8 ignore start */ ... /* c8 ignore stop */ directives around the lines 43-49 within index.ts since they're not testable with CJS.

rivy commented 1 year ago

@bcoe , CI now passes with the added /* c8 ignore start */ ... /* c8 ignore stop */ directives.

rivy commented 1 year ago

@bcoe , review?

rivy commented 1 year ago

@bcoe , once this is merged, it and yargs/y18n#147 can be used within yargs, which should fix all Deno warnings (excepting the escalade lack of explicit versioning, ref: https://github.com/lukeed/escalade/pull/8 and https://github.com/yargs/yargs/pull/2217#issuecomment-1322970494).

When done, I'll close yargs ~ PR #2216 and just fork and publish a yargs version for Deno with a fix for the escalade problem.

rivy commented 1 year ago

@bcoe , just checking back in... I believe all CI should PASS.

rivy commented 1 year ago

Ping @bcoe.

shadowspawn commented 1 year ago

I have not reviewed this PR but noticed the dependency updates. Just noting I have added a couple of other PRs with approaches to updating dependencies: #478 and #471

rivy commented 9 months ago

☠️ ⚰️ It's been a year; I'll move on with a fork containing the needed fixes.