Skip to content
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

Dependency on @isaacs/cliui@8.0.2 is broken #5

Closed
yedidyak opened this issue May 2, 2023 · 8 comments
Closed

Dependency on @isaacs/cliui@8.0.2 is broken #5

yedidyak opened this issue May 2, 2023 · 8 comments

Comments

@yedidyak
Copy link

yedidyak commented May 2, 2023

Hi, the dependency added here: https://github.com/isaacs/jackspeak/blame/main/package.json#L70 yesterday is on your fork @isaacs/cliui version 8.0.2. However, in the github there the latest is 8.0.1, and 8.0.2 has a set of dependencies that is different from those listed in github - and not available on npm as far as I can see. string-width in 8.0.1 and github, string-width-cjs in npm 8.0.2 etc.

Is this intentional?

Thank you!

@isaacs
Copy link
Owner

isaacs commented May 2, 2023

the published version in npm is 8.0.2, and the version on github is 8.0.2.

isaacs/cliui@aa397fe

Yes, very intentional, waiting for a fix PR to land, and then I'll switch jackspeak back to the mainline cliui.

What's broken about it?

@isaacs isaacs closed this as completed May 2, 2023
@antti-manninen-vmv
Copy link

antti-manninen-vmv commented May 3, 2023

I noticed the same.

If I understood correctly, @isaacs/cliui renames string-width 4.x.y to string-width-cjs with this alias syntax.

"string-width-cjs": "npm:string-width@^4.2.0",

It also depends on string-width 5.x.y that is installed as string-width.

Now other packages requiring "string-width" ends up requiring string-width 5.x.y that is pure ESM package and it breaks them even when they have defined that string-width should be version 4.x.y.

I guess npm/yarn should recognize that "string-width" folder under node_modules is 5.x.y and add string-width 4.x.y under node_modules/another-package/node_modules but it does not happen.

This problem started after jackspeak update.

Excerpt from my yarn.lock where wide-align requires wrong string-width.

wide-align@^1.1.2:
  version "1.1.5"
  resolved "https://registry.npmjs.org/wide-align/-/wide-align-1.1.5.tgz#df1d4c206854369ecf3c9a4898f1b23fbd9d15d3"
  integrity sha512-eDMORYaPNZ4sQIuuYPDHdQvf4gyCF9rEEV/yPxGfwPkRodwEgiMUUXTx/dex+Me0wxx53S+NgUHaP7y3MGlDmg==
  dependencies:
    string-width "^1.0.2 || 2 || 3 || 4"

"string-width-cjs@npm:string-width@^4.2.0", "string-width@^1.0.2 || 2 || 3 || 4", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
  name string-width-cjs
  version "4.2.3"
  resolved "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz#269c7117d27b05ad2e536830a8ec895ef9c6d010"
  integrity sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==
  dependencies:
    emoji-regex "^8.0.0"
    is-fullwidth-code-point "^3.0.0"
    strip-ansi "^6.0.1"

string-width@^5.0.1, string-width@^5.1.2:
  version "5.1.2"
  resolved "https://registry.npmjs.org/string-width/-/string-width-5.1.2.tgz#14f8daec6d81e7221d2a357e668cab73bdbca794"
  integrity sha512-HnLOCR3vjcY8beoNLtcjZ5/nxn2afmME6lhrDrebokqMap+XbeW8n9TXpPDOqdGK5qcI3oT0GKTW6wC7EMiVqA==
  dependencies:
    eastasianwidth "^0.2.0"
    emoji-regex "^9.2.2"
    strip-ansi "^7.0.1"

Now all you have to do to break the build is to include @isaacs/cliui as dependency to your package.json or alternatively run yarn upgrade as so many packages depends on jackspeak.


Build output:

var stringWidth = require('string-width')
                  ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /stuff/node_modules/string-width/index.js from /stuff/node_modules/wide-align/align.js not supported.
Instead change the require of index.js in /stuff/node_modules/wide-align/align.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/stuff/node_modules/wide-align/align.js:2:19)
    at Object.<anonymous> (/stuff/node_modules/gauge/render-template.js:2:13)
    at Object.<anonymous> (/stuff/node_modules/gauge/plumbing.js:3:22)
    at Object.<anonymous> (/stuff/node_modules/gauge/index.js:2:16)
    at Object.<anonymous> (/stuff/node_modules/npmlog/log.js:3:13)
    at Object.<anonymous> (/stuff/node_modules/@mapbox/node-pre-gyp/lib/node-pre-gyp.js:23:13)
    at Object.<anonymous> (/stuff/node_modules/@mapbox/node-pre-gyp/lib/main.js:9:22)
    at Object.<anonymous> (/stuff/node_modules/@mapbox/node-pre-gyp/bin/node-pre-gyp:4:1) {

@isaacs
Copy link
Owner

isaacs commented May 3, 2023

If I understood correctly, @isaacs/cliui renames string-width 4.x.y to string-width-cjs with this alias syntax.

That is correct. That's how it works.

Now other packages requiring "string-width" ends up requiring string-width 5.x.y that is pure ESM package and it breaks them even when they have defined that string-width should be version 4.x.y.

Those packages should define string-width as a dependency and specify the version they require, if they are using it as a dependency. If they are implicitly using whatever version of a transitive dep happens to show up in the tree, then that is a bug in those dependencies that was always bound to break eventually.

I guess npm/yarn should recognize that "string-width" folder under node_modules is 5.x.y and add string-width 4.x.y under node_modules/another-package/node_modules but it does not happen.

That dependency nesting is exactly what npm has done since npm version 1. (Though package aliases started being supported in npm v6, so prior to that it'd fail to recognize the specifier and not install at all.) The minimum node version that these packages support is 14, which ships with npm 7. The current npm release is version 9. None of this is new. Package aliases are 5 years old, and dependency nesting is 10 years old.

Does yarn not support package aliases, or nesting dependencies to avoid version conflicts? I was under the impression that it did.

If it does not, that sounds like a huge shortcoming of yarn. I recommend using npm if that is the case. (I mean, I recommend using npm anyway, it's the package manager I spent half my career working on lol)

@antti-manninen-vmv
Copy link

Thanks for fast reply! There was not any problems with your packages. Sorry for trouble and thanks for the support!

I first tested it with npm and it worked. Then I tested again with yarn but first removing existing yarn.lock file. It worked also.

So the problem was that yarn upgrade could not handle this change and resulted in situation described above.

@isaacs
Copy link
Owner

isaacs commented May 3, 2023

Ah, ok. I hope you saved that yarn lock file, it sounds like a bug worth reporting to them.

@antti-manninen-vmv
Copy link

I'll add here the whole solution if someone comes here with same problem.

I made some further testing and it seems that yarn works properly without yarn.lock file. When yarn.lock file exists, it handles dependencies incorrectly as I described above.

Problem looks like the same as described here:
yarnpkg/yarn#4812

I fixed the problem as proposed in that link. Basically hardcoding jackspeak to older version. The solution is not optimal but enables me to continue.

  "resolutions": {
    "jackspeak": "2.1.1"
  },

@isaacs
Copy link
Owner

isaacs commented May 4, 2023

Thanks for the update. That does seem like a pretty significant bug in yarns resolution algorithm. As more packages try to move step by step into esm support, by providing hybrid esm/cjs support, in a world where JavaScript's prolific author sindersorhus has dropped support for cjs in most of his libraries, I suspect this will be an increasingly frequent experience.

@danepowell
Copy link

Thanks for documenting the issue so thoroughly. In our case, upgrading to Yarn 3 fixed this.

fannheyward referenced this issue in neoclide/coc.nvim Sep 1, 2023
jerone added a commit to jerone/eslint-plugin-angular-template-consistent-this that referenced this issue Sep 17, 2023
* Add support for Angular 16.
* Rewritten CI testing for multiple NodeJS, ESLint and Angular version.
* Fix linting for lockfile. See lirantal/lockfile-lint#175 & isaacs/jackspeak#5
* Update all packages and run linting & Prettier.
* [Fix SonarCloud warning about duplicates in regex.](https://sonarcloud.io/project/issues?open=AYJ_Asgte7FPvNTvNf5F&id=jerone_eslint-plugin-angular-template-consistent-this)
* Connect SonarLint VSCode extension.
eamodio added a commit to eamodio/eslint-lite-webpack-plugin that referenced this issue Dec 1, 2023
Avoids minification
Avoids issue with yarn and npm aliases:
 - isaacs/jackspeak#5 (comment)
 - yarnpkg/yarn#4812
birtles added a commit to birchill/hikibiki-app that referenced this issue Jan 8, 2024
See: isaacs/jackspeak#5
     storybookjs/storybook#22431

Basically, it's a yarn bug and we should update to yarn 3 but for now
this will get GitHub CI to work.

(It actually works locally but maybe I'm lucky or maybe we have a more
recent version of yarn than GitHub Actions.)
birtles added a commit to birchill/hikibiki-app that referenced this issue Jan 8, 2024
See: isaacs/jackspeak#5
     storybookjs/storybook#22431

Basically, it's a yarn bug and we should update to yarn 3 but for now
this will get GitHub CI to work.

(It actually works locally but maybe I'm lucky or maybe we have a more
recent version of yarn than GitHub Actions.)
byCedric added a commit to expo/expo that referenced this issue Jan 17, 2024
…saacs/cliui` module aliases (#26459)

# Why

There are issues with yarn v1 and module aliases, as defined in
`@isaacs/cliui@8.0.2`
([issue](isaacs/jackspeak#5)). This package
adds support for both ESM and CJS through an alias:

```json package.json
{
  "dependencies": {
    "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"
  }
}
```

This doesn't work well with yarn v1 and causes issues like:

```
const stringWidth = require('string-width');
                    ^
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/expo/workingdir/build/updates-e2e/node_modules/string-width/index.js from /Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in /Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs:291:21)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/yargs/build/index.cjs:1:60678)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/yargs/index.cjs:5:30)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/react-native/scripts/generate-provider-cli.js:24:15) {
  code: 'ERR_REQUIRE_ESM'
}
Node.js v18.18.0
```

# How

The full dependency chain is:

-
[`@expo/config@8`](https://cdn.jsdelivr.net/npm/@expo/config@8/package.json)
|
[`@expo/metro-config@0.10`](https://cdn.jsdelivr.net/npm/@expo/metro-config@0.10/package.json)
-
[`sucrase@^3.35.0`](https://cdn.jsdelivr.net/npm/sucrase@%5E3.35.0/package.json)
-
[`glob@^10.3.10`](https://cdn.jsdelivr.net/npm/glob@%5E10.3.10/package.json)
-
[`jackspeak@^2.3.5`](https://cdn.jsdelivr.net/npm/jackspeak@%5E2.3.5/package.json)
-
[`@isaacs/cliui@^8.0.2`](https://cdn.jsdelivr.net/npm/@isaacs/cliui@^8.0.2/package.json)
      

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
byCedric added a commit to expo/expo that referenced this issue Jan 17, 2024
…saacs/cliui` module aliases (#26459)

# Why

There are issues with yarn v1 and module aliases, as defined in
`@isaacs/cliui@8.0.2`
([issue](isaacs/jackspeak#5)). This package
adds support for both ESM and CJS through an alias:

```json package.json
{
  "dependencies": {
    "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"
  }
}
```

This doesn't work well with yarn v1 and causes issues like:

```
const stringWidth = require('string-width');
                    ^
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/expo/workingdir/build/updates-e2e/node_modules/string-width/index.js from /Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in /Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/cliui/build/index.cjs:291:21)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/yargs/build/index.cjs:1:60678)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/yargs/index.cjs:5:30)
    at Object.<anonymous> (/Users/expo/workingdir/build/updates-e2e/node_modules/react-native/scripts/generate-provider-cli.js:24:15) {
  code: 'ERR_REQUIRE_ESM'
}
Node.js v18.18.0
```

# How

The full dependency chain is:

-
[`@expo/config@8`](https://cdn.jsdelivr.net/npm/@expo/config@8/package.json)
|
[`@expo/metro-config@0.10`](https://cdn.jsdelivr.net/npm/@expo/metro-config@0.10/package.json)
-
[`sucrase@^3.35.0`](https://cdn.jsdelivr.net/npm/sucrase@%5E3.35.0/package.json)
-
[`glob@^10.3.10`](https://cdn.jsdelivr.net/npm/glob@%5E10.3.10/package.json)
-
[`jackspeak@^2.3.5`](https://cdn.jsdelivr.net/npm/jackspeak@%5E2.3.5/package.json)
-
[`@isaacs/cliui@^8.0.2`](https://cdn.jsdelivr.net/npm/@isaacs/cliui@^8.0.2/package.json)
      

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
filips123 added a commit to filips123/PWAsForFirefox that referenced this issue Jan 22, 2024
Details:
- isaacs/jackspeak#5
- storybookjs/storybook#22431

In the future, we should migrate to Yarn Modern, npm or pnpm.
santa512 added a commit to santa512/PWAforFirefox that referenced this issue Mar 4, 2024
Details:
- isaacs/jackspeak#5
- storybookjs/storybook#22431

In the future, we should migrate to Yarn Modern, npm or pnpm.
mnvr added a commit to ente-io/ente that referenced this issue May 1, 2024
daniellockyer added a commit to TryGhost/Ghost that referenced this issue May 9, 2024
fixes #20065
refs storybookjs/storybook#22431 (comment)
refs isaacs/jackspeak#5

- this adds a resolution for jackspeak, which is a dependency deep
  inside storybook
- somewhere deep in the dependency tree, we're requiring an ESM package
  from a CJS package, which is causing issues
- this should hopefully avoid that by pinning jackspeak to a working
  version
daniellockyer added a commit to TryGhost/Ghost that referenced this issue May 9, 2024
fixes #20065
refs storybookjs/storybook#22431 (comment)
refs isaacs/jackspeak#5

- this adds a resolution for jackspeak, which is a dependency deep
  inside storybook
- somewhere deep in the dependency tree, we're requiring an ESM
  package from a CJS package, which is causing issues
- this should hopefully avoid that by pinning jackspeak to a working version
daniellockyer added a commit to TryGhost/Ghost that referenced this issue May 9, 2024
fixes #20065
refs storybookjs/storybook#22431 (comment)
refs isaacs/jackspeak#5

- this adds a resolution for jackspeak, which is a dependency deep
  inside storybook
- somewhere deep in the dependency tree, we're requiring an ESM
  package from a CJS package, which is causing issues
- this should hopefully avoid that by pinning jackspeak to a working version
ferrine added a commit to ferrine/phoenix-tutorial that referenced this issue May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants