fix: handle strings the same in cjs, esm, and deno

[isaacs]

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: #138

[isaacs]

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.

[isaacs]

Is nyc used when testing this library? I left the Istanbul comments in, but maybe they should be removed?

[isaacs]

Failure is from the standardx linter, will fix shortly.

[shadowspawn]

Switched from nyc to c8 in #80, so I think the comments can go.

[isaacs]

Hm, the standardx complaint is kind of confusing. It seems like it doesn't like import() statements, but they're clearly valid and supported. I tried declaring it as a global, but doesn't seem to have any effect. Any ideas? I'm not very familiar with standard or eslint, been all in on prettier in my projects for a long time now.

[shadowspawn]

You have the file named for explicit CommonJS (.cjs): cjs-esm-compare.cjs

Given you want to get access to both implementations and using require+import, should that be .mjs?

Update: well, tried that and didn't help!

[shadowspawn]

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 cjs-esm-compare.mjs and moved down into esm folder. Added to test:esm.

    "test:esm": "c8 mocha ./test/esm/*.mjs",

Reworked to use imports consistently:

'use strict'

/* global describe, it */
import cliuiESM from '../../index.mjs';
import cliuiCJS from '../../build/index.cjs'
import { expect } from 'chai';

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>...`

describe('consistent wrapping', () => {
  it('should produce matching output in cjs and esm', () => {
    const uiCJS = cliuiCJS({})
    const uiESM = cliuiESM({})
    uiCJS.div({
      padding: [0, 0, 0, 0],
      text,
    })
    uiESM.div({
      padding: [0, 0, 0, 0],
      text,
    })
    expect(uiCJS.toString()).to.equal(uiESM.toString())
  })
})

Then npm run test:esm runs the new test and it passes, but the old test fails (which might be expected since wrapping has changed). Running the test on its own:

% npx mocha test/esm/cjs-esm-compare.mjs

  consistent wrapping
    ✔ should produce matching output in cjs and esm

  1 passing (6ms)

[isaacs]

You have the file named for explicit CommonJS (.cjs): cjs-esm-compare.cjs

Yes, dynamic import() is now CommonJS node modules load ESM modules.

I suppose it could be refactored to be a .mjs test, and then load the cjs one with createRequire().

[isaacs]

Ah, didn't notice test:esm.

Updated the existing esm test to verify the correct behavior, instead of verifying the incorrect behavior. Should be passing now, everything looks good locally.

[isaacs]

Ah, needed to update deno test as well.

[shadowspawn]

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

``` # OLD % npm pack # master npm notice 📦 cliui@8.0.1 npm notice npm notice 📦 cliui@8.0.1 npm notice === Tarball Contents === npm notice 731B LICENSE.txt npm notice 3.0kB README.md npm notice 10.0kB build/index.cjs npm notice 1.1kB build/index.d.cts npm notice 9.7kB build/lib/index.js npm notice 1.0kB build/lib/string-utils.js npm notice 309B index.mjs npm notice 2.0kB package.json npm notice === Tarball Details === npm notice name: cliui npm notice version: 8.0.1 npm notice filename: cliui-8.0.1.tgz npm notice package size: 6.4 kB npm notice unpacked size: 27.7 kB npm notice shasum: b1a4ebefef09e89647b03e97b38bb5ad51ad77ba npm notice integrity: sha512-54py6xQmmLGZW[...]wTZmWq1VmXvRw== npm notice total files: 8 npm notice % npm i /Users/john/Documents/Sandpits/yargs/cliui/my-fork/cliui-8.0.1.tgz added 10 packages, and audited 11 packages in 1s % du -d=1 -h node_modules 312K node_modules % npm ls --all old@1.0.0 /Users/john/Documents/Sandpits/play/try-cliui/old └─┬ cliui@8.0.1 ├─┬ string-width@4.2.3 │ ├── emoji-regex@8.0.0 │ ├── is-fullwidth-code-point@3.0.0 │ └── strip-ansi@6.0.1 deduped ├─┬ strip-ansi@6.0.1 │ └── ansi-regex@5.0.1 └─┬ wrap-ansi@7.0.0 ├─┬ ansi-styles@4.3.0 │ └─┬ color-convert@2.0.1 │ └── color-name@1.1.4 ├── string-width@4.2.3 deduped └── strip-ansi@6.0.1 deduped ``` ``` # NEW % npm pack # isaacs-isaacs/esm-cjs-consistency npm notice npm notice 📦 cliui@8.0.1 npm notice === Tarball Contents === npm notice 731B LICENSE.txt npm notice 3.0kB README.md npm notice 10.4kB build/index.cjs npm notice 1.1kB build/index.d.cts npm notice 10.1kB build/lib/index.js npm notice 299B index.mjs npm notice 2.2kB package.json npm notice === Tarball Details === npm notice name: cliui npm notice version: 8.0.1 npm notice filename: cliui-8.0.1.tgz npm notice package size: 6.1 kB npm notice unpacked size: 27.7 kB npm notice shasum: 20504c591d90743add1b812cf439b9d845108468 npm notice integrity: sha512-F9qOs1ByD8UAg[...]uVxRAh/gdtH3g== npm notice total files: 7 npm notice % npm i /Users/john/Documents/Sandpits/yargs/cliui/my-fork/cliui-8.0.1.tgz added 23 packages, and audited 24 packages in 5s % du -d=1 -h node_modules 756K node_modules % npm ls --all new@1.0.0 /Users/john/Documents/Sandpits/play/try-cliui/new └─┬ cliui@8.0.1 ├─┬ string-width-cjs@npm:string-width@4.2.3 │ ├── emoji-regex@8.0.0 │ ├── is-fullwidth-code-point@3.0.0 │ └─┬ strip-ansi@6.0.1 │ └── ansi-regex@5.0.1 ├─┬ string-width@5.1.2 │ ├── eastasianwidth@0.2.0 │ ├── emoji-regex@9.2.2 │ └── strip-ansi@7.0.1 deduped ├─┬ strip-ansi-cjs@npm:strip-ansi@6.0.1 │ └── ansi-regex@5.0.1 ├─┬ strip-ansi@7.0.1 │ └── ansi-regex@6.0.1 ├─┬ wrap-ansi-cjs@npm:wrap-ansi@7.0.0 │ ├─┬ ansi-styles@4.3.0 │ │ └─┬ color-convert@2.0.1 │ │ └── color-name@1.1.4 │ ├─┬ string-width@4.2.3 │ │ ├── emoji-regex@8.0.0 │ │ ├── is-fullwidth-code-point@3.0.0 deduped │ │ └── strip-ansi@6.0.1 deduped │ └─┬ strip-ansi@6.0.1 │ └── ansi-regex@5.0.1 └─┬ wrap-ansi@8.1.0 ├── ansi-styles@6.2.1 ├── string-width@5.1.2 deduped └── strip-ansi@7.0.1 deduped ```

[isaacs]

Anything else needing to be done to land this? Some folks getting upset at me using a git dep while waiting.

[isaacs]

(Apparently the build fails when installing learna for some reason.)

[shadowspawn]

Anything else needing to be done to land this?

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.)

Some folks getting upset at me using a git dep while waiting.

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.)

[isaacs]

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

[OpportunityLiu]

The "string-width-cjs": "npm:string-width@^4.2.0" part breaks in yarn v1. While other packages still deps "string-width@^4", yarn install only one string-width-cjs in root of node_modules.

"string-width-cjs@npm:string-width@^4.2.0", string-width@^4.1.0, string-width@^4.2.0, string-width@^4.2.3:
  version "4.2.3"
  resolved "https://registry.npmmirror.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"

> 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 "string-width@5.1.2"
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 "string-width-cjs@4.2.3"
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.

[shadowspawn]

What other package can I install to reproduce the problem @OpportunityLiu , and what command can I run to see something break?

I tried installing just cliui using yarn@1 from a local pack file and using cliui from CommonJS and ESM without problem. But you did say it was problem with "other packages". I don't know yarn or web-ext to be sure what to look for, or to understand the problem from the "yarn why" commands. (Thanks for the detail though!)

[OpportunityLiu]

package.json

{
  "devDependencies": {
    "rimraf": "5.0.0",
    "web-ext": "7.6.2",
    "yargs": "17.7.2"
  }
}

It works if you install with no lock file, but after restore with yarn install --frozen-lockfile, some packages does not work.

C:\Users\lzy\Documents\Sources\node-test> yarn install
yarn install v1.22.19
warning package.json: No license field
info No lockfile found.
warning No license field
[1/4] Resolving packages...
warning web-ext > sign-addon > request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
warning web-ext > sign-addon > request > uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
warning web-ext > sign-addon > request > har-validator@5.1.5: this library is no longer supported
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
Done in 16.53s.
C:\Users\lzy\Documents\Sources\node-test> node -r ansi-align
Welcome to Node.js v16.20.0.      
Type ".help" for more information.
>
(To exit, press Ctrl+C again or Ctrl+D or type .exit)
>
C:\Users\lzy\Documents\Sources\node-test> rm -Force -Recurse .\node_modules\
C:\Users\lzy\Documents\Sources\node-test> yarn install --frozen-lockfile
yarn install v1.22.19
warning package.json: No license field
warning No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
warning Pattern ["strip-ansi@^6.0.1"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-strip-ansi-cjs-6.0.1-9e26c63d30f53443e9489495b2105d37b67a85d9-integrity\\node_modules\\strip-ansi-cjs" as pattern ["strip-ansi-cjs@npm:strip-ansi@^6.0.1"]. This could result in non-deterministic behavior, skipping.
warning Pattern ["string-width@^4.1.0"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-string-width-cjs-4.2.3-269c7117d27b05ad2e536830a8ec895ef9c6d010-integrity\\node_modules\\string-width-cjs" as pattern ["string-width-cjs@npm:string-width@^4.2.0"]. This could result in non-deterministic behavior, skipping.
warning Pattern ["strip-ansi@^6.0.0"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-strip-ansi-cjs-6.0.1-9e26c63d30f53443e9489495b2105d37b67a85d9-integrity\\node_modules\\strip-ansi-cjs" as pattern ["strip-ansi-cjs@npm:strip-ansi@^6.0.1"]. This could result in non-deterministic behavior, skipping.
warning Pattern ["string-width@^4.2.3"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-string-width-cjs-4.2.3-269c7117d27b05ad2e536830a8ec895ef9c6d010-integrity\\node_modules\\string-width-cjs" as pattern ["string-width-cjs@npm:string-width@^4.2.0"]. This could result in non-deterministic behavior, skipping.
warning Pattern ["string-width@^4.2.0"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-string-width-cjs-4.2.3-269c7117d27b05ad2e536830a8ec895ef9c6d010-integrity\\node_modules\\string-width-cjs" as pattern ["string-width-cjs@npm:string-width@^4.2.0"]. This could result in non-deterministic behavior, skipping.
warning Pattern ["wrap-ansi@^7.0.0"] is trying to unpack in the same destination "C:\\Users\\lzy\\Storage\\Data\\yarn\\cache\\v6\\npm-wrap-ansi-cjs-7.0.0-67e145cff510a6a6984bdf1152911d69d2eb9e43-integrity\\node_modules\\wrap-ansi-cjs" as pattern ["wrap-ansi-cjs@npm:wrap-ansi@^7.0.0"]. This could result in non-deterministic behavior, skipping.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 12.60s.
C:\Users\lzy\Documents\Sources\node-test> node -r ansi-align
C:\Users\lzy\Documents\Sources\node-test\node_modules\ansi-align\index.js:3
const stringWidth = require('string-width')
                    ^

Error [ERR_REQUIRE_ESM]: require() of ES Module C:\Users\lzy\Documents\Sources\node-test\node_modules\string-width\index.js from C:\Users\lzy\Documents\Sources\node-test\node_modules\ansi-align\index.js 
not supported.
Instead change the require of C:\Users\lzy\Documents\Sources\node-test\node_modules\string-width\index.js in C:\Users\lzy\Documents\Sources\node-test\node_modules\ansi-align\index.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (C:\Users\lzy\Documents\Sources\node-test\node_modules\ansi-align\index.js:3:21) {
  code: 'ERR_REQUIRE_ESM'
}

[OpportunityLiu]

Here's my action https://github.com/EhTagTranslation/EhSyringe/actions/runs/4900000993/jobs/8752270422

[isaacs]

Seems like the same as this issue: https://github.com/isaacs/jackspeak/issues/5

This is a yarn bug, I think. I'm not sure what the best workaround is, but package aliases ought to be supported and sometimes it gets confused by them.

[shadowspawn]

Playing with an idea.

We are using rollup to generate the cjs anyway. Could possibly use rollup to include the older dependencies directly so the npm aliases are only a dev dependency and bypass the yarn problem. But lose dedupe with other installs.

This config generates a self-contained file to demonstrate idea, but the output file includes multiple copies of some dependencies. (I didn't work out the TypeScript/rollup interaction to tune the config better.)

rollup.config.js

import ts from 'rollup-plugin-ts'
import { nodeResolve } from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';

const output = {
  format: 'cjs',
  file: './build/index.cjs',
  exports: 'default'
}

if (process.env.NODE_ENV === 'test') output.sourcemap = true

export default {
  input: './lib/cjs.ts',
  output,
  plugins: [
    ts({ /* options */ }),
    nodeResolve(),
    commonjs({ extensions: ['.ts', '.js'], transformMixedEsModules: true }),
  ]
}

[shadowspawn]

@isaacs Does using a CommonJS implementation in ESM work for clients of jackspeak?

You mentioned createRequire in https://github.com/yargs/cliui/issues/138#issuecomment-1528855803 so might be ok, but you may have decided since then that really want "true ESM"!

Related: In older discussion, Deno and Yargs browser support were the scenarios of concern for "true ESM": https://github.com/yargs/cliui/issues/89#issuecomment-1537299183

[isaacs]

"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, import * from cliui should be functionally equivalent to require('cliui'). However you wanna accomplish that is fine.

[shadowspawn]

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!

[naugtur]

When yarn3 attempts to run audit on a project with @isaacs/cliui in dependencies, it gets a HTTP400 from the audit API because of the *-cjs references:

https://github.com/isaacs/cliui/blame/main/package.json#L53

Repro with details: https://github.com/danjm/yarn-audit-bug/pull/1

[shadowspawn]

I wasn't able to reproduce this. I cloned out your repo, and ran (on Mac):

% yarn --version
3.2.4
% yarn install
...
% yarn npm audit
➤ YN0001: No audit suggestions

[shadowspawn]

Hang on, I was not on correct branch...

[shadowspawn]

I pulled from correct repo, but still can not reproduce. What commands are you running that are showing an issue?

[naugtur]

Sorry! You need to add --recursive because otherwise yarn won't send transitive dependencies. I'll update the description there.

[isaacs]

...?!

Yarn doesn't audit transitive deps by default??

[naugtur]

🤷‍♂️ c'est la vie

[shadowspawn]

Ok, reproduced now.

I have an alternative PR for cliui prepared, but don't have any yarns knowledge to try plugging it in to your repo and see if it performs better. If you can easily try this repo as a replacement for the code from this PR, I would love to know if avoids the problem. No worries if you are not interested!

• https://github.com/yargs/cliui/pull/143
• https://github.com/shadowspawn/cliui/tree/feature/esm-wrapping

[Bonnie-dot]

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.

[G-Rath]

@shadowspawn I've tried your PR and confirmed it doesn't have the same problem as @isaacs/cliui 🎉

[LennonReid]

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!

[shadowspawn]

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.

• https://github.com/yarnpkg/yarn/releases/tag/v1.22.22