-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle strings the same in cjs, esm, and deno #139
base: master
Are you sure you want to change the base?
Conversation
It seems a bit weird to double up the dependencies, but I guess if those modules were shipped as hybrid esm/cjs, it'd be the same amount of code anyway. The only hazard is that the esm versions will probably get bugfixes that aren't going to be backported, but all three of them seem extremely stable, so I think the risk is low. |
Is nyc used when testing this library? I left the Istanbul comments in, but maybe they should be removed? |
Failure is from the standardx linter, will fix shortly. |
Switched from nyc to c8 in #80, so I think the comments can go. |
Hm, the standardx complaint is kind of confusing. It seems like it doesn't like |
You have the file named for explicit CommonJS ( Given you want to get access to both implementations and using require+import, should that be Update: well, tried that and didn't help! |
Hacking about without a deep understanding yet, but got new test running. The esm tests are a bit loose as not covered by standardx yet. Renamed to
Reworked to use imports consistently:
Then % npx mocha test/esm/cjs-esm-compare.mjs
consistent wrapping
✔ should produce matching output in cjs and esm
1 passing (6ms) |
Yes, dynamic I suppose it could be refactored to be a |
Ah, didn't notice Updated the existing esm test to verify the correct behavior, instead of verifying the incorrect behavior. Should be passing now, everything looks good locally. |
- Use trimStart/trimEnd instead of deprecated trimLeft/trimRight
This also ports some of the `// istanbul ignore` comments to their associated `/* c8 ignore start/stop */` equivalents, and coverage-ignores some value fallbacks that are there for safety but can never be hit in normal usage. Fix: yargs#138
a7db817
to
8ee2fad
Compare
Previously, it was verifying *incorrect* behavior.
8ee2fad
to
f236663
Compare
Ah, needed to update deno test as well. |
For interest I had a look at the impact in package size. As might be expected, installed size about doubles since two of everything. 😅 Details
|
Anything else needing to be done to land this? Some folks getting upset at me using a git dep while waiting. |
(Apparently the build fails when installing learna for some reason.) |
Nothing else from you at this point from me anyway, thanks. I'll review it as a supply-both approach to getting wrapping working in esm et al. A high level question is whether we want to land a double-dependency update, and I'll want input from @bcoe on that. I know people have expressed concerns about number of dependencies and size of Yargs install in past so this is of some concern, which is why I checked install sizes for reference. (And Benjamin has also mentioned heading for ESM whether esm-first or esm-only at some point.)
I don't have a timeline for you. Could you work around the limitation in the meantime by using require from esm and getting the CommonJs implementation? (I am not sure whether this makes any sense for your runtime setup.) |
I'll just publish my fork as @isaacs/cliui for now. Was just being lazy. I don't mind littering my npm account, it's a garbage pile of throwaway junk after all these years anyway lol |
The
> yarn why string-width
yarn why v1.22.19
[1/4] Why do we have the module "string-width"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
- "web-ext#update-notifier#boxen" depends on it
- Hoisted from "web-ext#update-notifier#boxen#string-width"
- Hoisted from "web-ext#update-notifier#boxen#widest-line#string-width"
- Hoisted from "web-ext#update-notifier#boxen#wrap-ansi#string-width"
- Hoisted from "rimraf#glob#jackspeak#@isaacs#cliui#string-width"
info Disk size without dependencies: "200KB"
info Disk size with unique dependencies: "280KB"
info Disk size with transitive dependencies: "300KB"
info Number of shared dependencies: 4
Done in 0.54s.
> yarn why string-width-cjs
yarn why v1.22.19
[1/4] Why do we have the module "string-width-cjs"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
- "yargs" depends on it
- Hoisted from "yargs#string-width-cjs"
- Hoisted from "yargs#cliui#string-width-cjs"
- Hoisted from "web-ext#yargs#string-width-cjs"
- Hoisted from "yargs#cliui#wrap-ansi-cjs#string-width-cjs"
- Hoisted from "web-ext#addons-linter#yargs#string-width-cjs"
- Hoisted from "rimraf#glob#jackspeak#@isaacs#cliui#string-width-cjs"
- Hoisted from "web-ext#update-notifier#boxen#ansi-align#string-width-cjs"
info Disk size without dependencies: "84KB"
info Disk size with unique dependencies: "164KB"
info Disk size with transitive dependencies: "184KB"
info Number of shared dependencies: 4
Done in 0.55s. |
@isaacs Does using a CommonJS implementation in ESM work for clients of jackspeak? You mentioned Related: In older discussion, Deno and Yargs browser support were the scenarios of concern for "true ESM": #89 (comment) |
"True ESM" is definitely the goal in principle, as eventually I would like tap to work in deno and browsers, so I've been shipping full hybrid builds for everything. That said, the stuff using cliui won't ever work in browsers per se, and most of the cli runner is pretty node specific, so getting to deno support will be a big lift as well, so just shipping CJS is fine, too. The main thing for me here is, |
Good info, thanks @isaacs The reason I am considering different approaches is that the underling lack of ansi support in ESM flavour of cliui generated a few issues across yargs in a couple of years. Using the npm aliases via jackspeak generated a couple of reports within days (due to breaking builds). Which we only know now because you tried an approach @isaacs , full credit for that! |
When yarn3 attempts to run audit on a project with https://github.com/isaacs/cliui/blame/main/package.json#L53 Repro with details: |
I wasn't able to reproduce this. I cloned out your repo, and ran (on Mac):
|
This comment was marked as outdated.
This comment was marked as outdated.
I pulled from correct repo, but still can not reproduce. What commands are you running that are showing an issue? |
Sorry! |
...?! Yarn doesn't audit transitive deps by default?? |
🤷♂️ c'est la vie |
Ok, reproduced now. I have an alternative PR for |
I meet the same issue with yarn. I went through all comments, but I didn't find any solution except using npm. And I used npm, it can works. Hopefully it can be solved in the future. |
@shadowspawn I've tried your PR and confirmed it doesn't have the same problem as |
I encountered the same issue since yesterday. However, after switching to PNPM, the problem was resolved. Running the command 'yarn why cliui' pointed me to the @ngneat/transloco. Thanks for the guidance; you all made my day! |
For yarn 1 users hitting this page because of runtime failures with cliui and string-width, try upgrading to yarn v1.22.22 or higher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a yarn 1 update available, I think this is now ok.
I have an alternative PR open which uses rollup plugins rather than a package manager alias to support the cjs/esm implementations, different tradeoffs but similar outcome: #143.
I do prefer approach in #143 and still getting people reporting issues from the breakage in yarn 1
"string-width": "^5.1.2", | ||
"string-width-cjs": "npm:string-width@^4.2.0", | ||
"strip-ansi": "^7.0.1", | ||
"strip-ansi-cjs": "npm:strip-ansi@^6.0.1", | ||
"wrap-ansi": "^8.1.0", | ||
"wrap-ansi-cjs": "npm:wrap-ansi@^7.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how helpful this is, but this change in @isaacs/cliui
fork scared me.
I'm running this on the Docusaurus repo, trying to detect possible supply chain attacks, and got these aliases being reported:
npx lockfile-lint --path yarn.lock --type yarn --allowed-hosts yarn --validate-https --validate-package-names
detected resolved URL for package with a different name: string-width-cjs
expected: string-width-cjs
actual: string-width
detected resolved URL for package with a different name: strip-ansi-cjs
expected: strip-ansi-cjs
actual: strip-ansi
detected resolved URL for package with a different name: wrap-ansi-cjs
expected: wrap-ansi-cjs
actual: wrap-ansi
✖ Error: security issues detected!
And there's this "anonymous" guy that published empty packages on npm with the exact same name:
https://www.npmjs.com/package/string-width-cjs
https://www.npmjs.com/package/strip-ansi-cjs
https://www.npmjs.com/package/wrap-ansi-cjs
I don't know how harmful it could be, but this looks suspicious that those packages even get a few weekly downloads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a security issue, it's just a dependency alias, which every js package manager should support by now. If yarn is complaining, it's likely a yarn bug.
Those packages likely get downloads because the registry is constantly being mirrored by many third-party registry instances. I don't know if they're malicious or just litter, but they're irrelevant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That a might be a false positive from lockfile-lint, possibly it does not look for package aliases. The author of lockfile-lint
may be interested.
If is interesting that there are some matching packages published. The optimistic view is perhaps someone was investigating working around the bug in earlier versions of yarn. The pessimistic view is someone was investigating exploiting the problem. The failures we have seen are a runtime failure rather than a download so not directly fixable/exploitable in this way. (But someone might possibly see the alias name in a message and think it was missing and try installing it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is definitely a false positive in whatever tool is generating this warning. I recommend reporting it to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isaacs considering that the npm registry itself is/was vulnerable to npm package aliases and allowed spoofing package names (the way you used them here in the package.json) is enough to warrant that this is a real-world concern and not theoretical, in my opinion. See here for evidence: https://snyk.io/blog/exploring-extensions-of-dependency-confusion-attacks-via-npm-package-aliasing/
@slorber lockfile-lint allows you to accept the risk of package aliases as long as you explicitly call them out, here's how to allow-list one of the packages:
npx lockfile-lint --path package-lock.json --allowed-hosts yarn npm --validate-https --validate-package-names --allowed-package-name-aliases string-width-cjs:string-width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lirantal That blog post is misleading. It's not a "supply chain security" issue, it's just a website display issue. I'm not saying it's not a bug, and yes it does potentially lead to falsely providing reputation in some way to the other package name, but it's imo quite a stretch to call it evidence of a supply chain security bug for package publishers or consumers using package aliases as they're intended. The website is not part of the supply chain, and the registry and all modern clients of it handle aliases just fine.
The failure of lockfile-lint
to do so as well leads to false positives like this one creating make-work and wasting everyone's time. Please reconsider.
This also ports some of the
// istanbul ignore
comments to theirassociated
/* c8 ignore start/stop */
equivalents, andcoverage-ignores some value fallbacks that are there for safety but can
never be hit in normal usage.
Fix: #138