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

Wrapping logic seems to be weirdly different between ESM vs CJS versions #138

Open
isaacs opened this issue Apr 29, 2023 · 19 comments · May be fixed by #139
Open

Wrapping logic seems to be weirdly different between ESM vs CJS versions #138

isaacs opened this issue Apr 29, 2023 · 19 comments · May be fixed by #139

Comments

@isaacs
Copy link

isaacs commented Apr 29, 2023

Example:

const cliuiCJS = require('cliui')
import('cliui').then(({ default: cliuiESM }) => {
  const uiCJS = cliuiCJS({})
  const uiESM = cliuiESM({})

  const text = `usage: git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]
               <tagname> [<commit> | <object>]
   or: git tag -d <tagname>...
   or: git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]
               [--points-at <object>] [--column[=<options>] | --no-column]
               [--create-reflog] [--sort=<key>] [--format=<format>]
               [--merged <commit>] [--no-merged <commit>] [<pattern>...]
   or: git tag -v [--format=<format>] <tagname>...`

  console.error('~ ~ ~ ~ ~ ~ ~ ~ ~ ~')
  console.error('cjs')
  uiCJS.div({
    padding: [0, 0, 0, 0],
    text,
  })
  console.log(uiCJS.toString())
  console.error('~ ~ ~ ~ ~ ~ ~ ~ ~ ~')
  console.error('esm')
  uiESM.div({
    padding: [0, 0, 0, 0],
    text,
  })
  console.log(uiESM.toString())
})

/* output:
~ ~ ~ ~ ~ ~ ~ ~ ~ ~
cjs
usage: git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]
<tagname> [<commit> | <object>]
or: git tag -d <tagname>...
or: git tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]
[--points-at <object>] [--column[=<options>] | --no-column]
[--create-reflog] [--sort=<key>] [--format=<format>]
[--merged <commit>] [--no-merged <commit>] [<pattern>...]
or: git tag -v [--format=<format>] <tagname>...
~ ~ ~ ~ ~ ~ ~ ~ ~ ~
esm
usage: git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] [-e]

       <tagname> [<commit> | <object>]
   or: git tag -d <tagname>...
   or: git
 tag [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]

  [--points-at <object>] [--column[=<options>] | --no-column]
               [--
create-reflog] [--sort=<key>] [--format=<format>]
               [--merged <comm
it>] [--no-merged <commit>] [<pattern>...]
   or: git tag -v [--format=<format>]
 <tagname>...
 */

I'm not sure why, but the esm results seem to wrap at very confusing places, differently from the cjs version.

@isaacs
Copy link
Author

isaacs commented Apr 29, 2023

Ah, it looks like the cjs version is setting up the Mixin with different values:

const stringWidth = require('string-width');
const stripAnsi = require('strip-ansi');
const wrap = require('wrap-ansi');
function ui(opts) {
    return cliui(opts, {
        stringWidth,
        stripAnsi,
        wrap
    });
}

Whereas the mjs does this:

import { wrap, stripAnsi } from './build/lib/string-utils.js'

export default function ui (opts) {
  return cliui(opts, {
    stringWidth: (str) => {
      return [...str].length
    },
    stripAnsi,
    wrap
  })
}

So the difference appears to be that the functions in string-utils.ts are not equivalent.

@isaacs
Copy link
Author

isaacs commented Apr 29, 2023

Confirmed, this fixes the issue:

diff --git a/index.mjs b/index.mjs
index bc7a022..ccb76e7 100644
--- a/index.mjs
+++ b/index.mjs
@@ -1,6 +1,8 @@
 // Bootstrap cliui with CommonJS dependencies:
 import { cliui } from './build/lib/index.js'
-import { wrap, stripAnsi } from './build/lib/string-utils.js'
+import { stripAnsi } from './build/lib/string-utils.js'
+import { createRequire } from 'module'
+const wrap = createRequire(import.meta.url)('wrap-ansi')
 
 export default function ui (opts) {
   return cliui(opts, {

I'm guessing the reason for not using wrap-ansi in this way is to support Deno and browsers, which do not have createRequire. But the wrap method in string-utils is not working properly, so there's a bug there.

It'd probably be best to ditch wrap-ansi entirely (including in the cjs version) and fix the wrap method in string-utils.ts, yes?

@isaacs
Copy link
Author

isaacs commented Apr 29, 2023

Ugh, and it'd be trivial to update strip-ansi, wrap-ansi, and string-width to ESM, but that would drop support for cjs, because those modules are ESM-only.

Can do it with a npm alias, idk if that's too weird, but it would at least fix the issue.

isaacs added a commit to isaacs/cliui that referenced this issue Apr 29, 2023
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
@isaacs isaacs linked a pull request Apr 29, 2023 that will close this issue
@shadowspawn
Copy link
Member

shadowspawn commented Apr 29, 2023

See also: #89

Related: yargs/yargs#2112 (comment)

@isaacs
Copy link
Author

isaacs commented Apr 30, 2023

Fixed by #139

isaacs added a commit to isaacs/cliui that referenced this issue May 1, 2023
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
isaacs added a commit to isaacs/jackspeak that referenced this issue May 1, 2023
@Netail
Copy link

Netail commented Mar 6, 2024

Any update on this? @shadowspawn (PR #143 ) Experiencing the following issue with jest;

Error [ERR_REQUIRE_ESM]: require() of ES Module /.../node_modules/string-width/index.js from /.../node_modules/cliui/build/index.cjs not supported.

@shadowspawn
Copy link
Member

No update

Interested in knowing more about your context to understand what the error message might be about, and whether the proposed PR will help. This issue is about cliui not wrapping when used from esm which is due to complications around string-width and cjs/esm but does not (normally) cause runtime import errors. Just lack of wrapping.

Is your project pulling in cliui (e.g. v8.0.1) or @isaacs/cliui (v8.0.2)? If your project is pulling in @isaacs/cliui, are you using Yarn v1? Do you have a public repo where I can try and reproduce?

@Netail
Copy link

Netail commented Mar 6, 2024

So our project uses jest for unit testing, where a lower level dependency uses string-width.

Error [ERR_REQUIRE_ESM]: require() of ES Module <dir>/node_modules/string-width/index.js from <dir>/node_modules/cliui/build/index.cjs not supported.
Instead change the require of index.js in <dir>/node_modules/cliui/build/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (<dir>/node_modules/cliui/build/index.cjs:291:21)
    at Object.<anonymous> (<dir>/node_modules/yargs/build/index.cjs:1:60678)
    at Object.<anonymous> (<dir>/node_modules/yargs/index.cjs:5:30)
    at _yargs (<dir>/node_modules/jest-cli/build/run.js:30:39)
    at buildArgv (<dir>/node_modules/jest-cli/build/run.js:149:26)
    at Object.run (<dir>/node_modules/jest-cli/build/run.js:124:24)
    at Object.<anonymous> (<dir>/node_modules/jest-cli/bin/jest.js:16:17)
    at Object.<anonymous> (<dir>/node_modules/jest/bin/jest.js:12:3)

I can create a small test environment

@Netail
Copy link

Netail commented Mar 6, 2024

I've created a test environment; https://github.com/Netail/jest-string-width
yarn install -> yarn test

@shadowspawn
Copy link
Member

shadowspawn commented Mar 6, 2024

Great, thanks @Netail , I'll give that a try in next couple of days.

(I can see in the yarn.lock both cliui and @isaacs/cliui, so first question answered already.)

@shadowspawn
Copy link
Member

Reproduced with Yarn 1. The Jest problem is triggered by the use of aliases in #139 which is published in @isaacs/cliui. Yarn is getting confused with the aliased (string-width-cjs) and unaliased (string-width) packages.

Details

I was able to get a working install by deleting the node_modules file and creating an empty yarn.lock file before installing with yarn 1.22.21, but I suspect it is fragile and not a reliable fix.

This output is from the example repo and an install with the original yarn.lock.

The only installed package that uses alias string-width-cjs is @isaacs/cliu. Yarn thinks multiple packages are using string-width-cjs, but of course they don't and in their code they require string-width and break when they get a newer esm-only version.

% yarn why string-width
yarn why v1.22.21
[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 "string-width@5.1.2"
info Reasons this module exists
   - "eslint-config-next#@next#eslint-plugin-next#glob#jackspeak#@isaacs#cliui" depends on it
   - Hoisted from "eslint-config-next#@next#eslint-plugin-next#glob#jackspeak#@isaacs#cliui#string-width"
   - Hoisted from "eslint-config-next#@next#eslint-plugin-next#glob#jackspeak#@isaacs#cliui#wrap-ansi#string-width"
info Disk size without dependencies: "220KB"
info Disk size with unique dependencies: "328KB"
info Disk size with transitive dependencies: "348KB"
info Number of shared dependencies: 4
✨  Done in 0.15s.
% yarn why string-width-cjs
yarn why v1.22.21
[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 "string-width-cjs@4.2.3"
info Reasons this module exists
   - "jest#jest-cli#yargs" depends on it
   - Hoisted from "jest#jest-cli#yargs#string-width-cjs"
   - Hoisted from "jest#jest-cli#yargs#cliui#string-width-cjs"
   - Hoisted from "eslint-config-next#@next#eslint-plugin-next#glob#jackspeak#@isaacs#cliui#string-width-cjs"
   - Hoisted from "jest#jest-cli#yargs#cliui#wrap-ansi-cjs#string-width-cjs"
info Disk size without dependencies: "20KB"
info Disk size with unique dependencies: "128KB"
info Disk size with transitive dependencies: "148KB"
info Number of shared dependencies: 4

The suspect entry in the yarn.lock file is:

"string-width-cjs@npm:string-width@^4.2.0", 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.yarnpkg.com/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"

@isaacs
Copy link
Author

isaacs commented Mar 8, 2024

@arcanis This is another example of the "yarn 1 deduplicating aliased packages incorrectly" issue.

@Netail I have not checked, but I'm willing to be that this would not be a problem with any other package manager. Try using npm, pnpm, or latest yarn (v4). Yarn 1 should not be used, as long as this bug prevents it from correctly resolving dependency graphs.

@Netail
Copy link

Netail commented Mar 9, 2024

When installing yarn (npm i -g yarn) it uses v1 by default. (Meaning a lot of people still use v1 and will potentially run into the same issue) Wouldn't it make more sense to update the string-width package, so it would work with v1?

@arcanis
Copy link

arcanis commented Mar 9, 2024

I tried to release a 1.22.22 release which should fix this issue. Running the reproduction in eslint/eslint#17215 (reply in thread) with yarn@1.22.22 instead of yarn@1 seems to work. Can you try to upgrade and let me know?

@shadowspawn
Copy link
Member

shadowspawn commented Mar 9, 2024

I retested reproduction from @Netail . Looks good with yarn@1.22.22.

I needed to delete the yarn.lock (which I expected) and then install worked as expected. Checking yarn why string-width looks tidy.

For reference, the yarn fix is: yarnpkg/yarn#9023

@shadowspawn
Copy link
Member

(I'll try a couple of different reproductions from #139 as well.)

@Netail
Copy link

Netail commented Mar 9, 2024

I retested reproduction from @Netail . Looks good with yarn@1.22.22.

I needed to delete the yarn.lock (which I expected) and then install worked as expected. Checking yarn why string-width looks tidy.

For reference, the yarn fix is: yarnpkg/yarn#9023

Yes seems like my issue has been resolved with v1.22.22, that's great! Thank you guys very much :)

@shadowspawn
Copy link
Member

I reproduced problems with packages used in #139 (comment) with yarn@1.22.21, and fixed with yarn@1.22.22. (rimraf and web-ext and yargs.)

@callmeteus
Copy link

Just for a feedback, I've had this problem and upgrading to yarn 1.22.22 as cited worked perfectly.

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

Successfully merging a pull request may close this issue.

5 participants