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

Reinstate smart whitespace-based wrapping for ESM? #89

Open
JaredReisinger opened this issue Sep 29, 2020 · 12 comments
Open

Reinstate smart whitespace-based wrapping for ESM? #89

JaredReisinger opened this issue Sep 29, 2020 · 12 comments

Comments

@JaredReisinger
Copy link

I've started migrating some of my personal projects to native ESM (not transpiled), and suddenly started noticing that string wrapping in the command-line help became "dumb", simply injecting \n at the column boundary. Digging into yargs and cliui, I found that the cliui index.mjs entrypoint is using some simple implementations for stripAnsi and wrap from lib/string-utils.ts, which contains a comment about being a minimal replacement for the older/existing CJS libraries.

Would it be possible to simply import those CJS libraries, even in an ESM context? I know this should work fine in Node.js, I'm not sure whether Deno adds additional restrictions. (But Deno appears to have its own entrypoint, so this may be a non-issue.)

Just for giggles, I hand-patched my local install in node_modules (for a yargs-using tool) so that cliui/index.mjs looked like:

import { cliui } from './build/lib/index.js'

import stringWidth from 'string-width';
import stripAnsi from 'strip-ansi';
import wrap from 'wrap-ansi';

export default function ui (opts) {
  return cliui(opts, {
    stringWidth,
    stripAnsi,
    wrap
  })
}

(basically, a naive merge of the existing wrapper and the TS/CJS index)... and it seemed to work perfectly (on Node.js v14.5.0), giving me pretty word-wrapping, and no weird clipping.

current

  -v, --verbose  Increase verbosity of console output, can be given multiple time
                 es to increase verbosity                                [count]

patched

  -v, --verbose  Increase verbosity of console output, can be given multiple
                 times to increase verbosity                             [count]

Is there any reason cliui can't simply leverage the existing width/ansi/wrap CJS libraries for ESM? I'm happy to provide this as a PR with updated ESM tests if that would help.

@bcoe
Copy link
Member

bcoe commented Oct 1, 2020

@JaredReisinger my intended goal was a true ESM module, with the benefits that go with that, such as working in the browser and Deno.

I can't see why we couldn't provide the CJS modules for ESM, as long as we don't break the logic for Deno or yargs' browser support -- the better future would potentially be that we implement "smarter" logic ourselves, so that we get the behavior in Deno too.

@JaredReisinger
Copy link
Author

I applaud the goal of a true/pure ESM module; I hadn't considered cliui (and yargs) for use in the browser! I'm in no real rush for a prettier word-wrap for my personal ESM command-line tools, so I won't push for the above workaround in the short-term. (Perhaps I'll see about adding ESM support into those now-CJS libraries, if they'd be amenable, or maybe even publish some ESM-only equivalents myself.)

@bcoe
Copy link
Member

bcoe commented Oct 1, 2020

@JaredReisinger yeah, I'd be happy to pull them in and use them as ESM deps, perhaps we could nudge the projects towards publishing to Deno too.

It's a bit of a pain in the neck, but it creates a bridge for the ecosystem to consider using imports.

@aral
Copy link

aral commented Nov 18, 2020

I’m not sure if this is directly related to this issue (feels like it) but when wrapping my app with Nexe, I’m getting the following error:

Error: Cannot find module 'string-width'
Require stack:
- /usr/local/bin/node_modules/cliui/build/index.cjs 
…

Seems Nexe’s resolver is not able to find that module and bundle it in automatically.

Update: I think the issue is that Nexe’s dependency graph doesn’t handle exports maps: nexe/nexe#834

@bcoe
Copy link
Member

bcoe commented Nov 23, 2020

@aral this thread is related to a visual difference that occurs on Deno, because I shimmed the cliui dependency. Looks like you're on the right track upstream.

@isaacs
Copy link

isaacs commented Apr 30, 2023

Fixed by #139

@shadowspawn
Copy link
Member

#89 (comment)

my intended goal was a true ESM module, with the benefits that go with that, such as working in the browser and Deno.

I can't see why we couldn't provide the CJS modules for ESM, as long as we don't break the logic for Deno or yargs' browser support

Things have changed in a couple of years, but we still have the same outcome that we can't conveniently have a "true ESM module" that works in CJS and is full featured in both.

If we abandon the "true ESM", then we can potentially use the older versions of string-width and strip-ansi and wrap-ansi in CJS and ESM-using-CJS, and the newer versions via URL for Deno and browser.

Demo support is implemented by having a separate entry point. The dependencies are by URL, and so I assume we could use the newer esm-only version of the modules.

Browser support in Yargs uses a separate entry point, and again uses URL. If we used the same pattern here then we again have the newer dependencies in URL and independent. (Albeit it isn't certain browser support in Yargs is still functional: yargs/yargs#1981)

I think it is fragile having some dependencies in package.json and some in URL's in files, but it is a trade-off to support more runtime environments with the current state of play! If it is good enough for Yargs, then good enough for cliui too?

@shadowspawn
Copy link
Member

shadowspawn commented May 13, 2023

I have been thinking about how to group the client scenarios. I think most of the likely client use cases are covered by:

  • CommonJS
  • ESM with support for importing CommonJS, like node context
  • Deno
  • "browser"
    • do not use node libraries
    • do not import CommonJS
    • import external modules using an URL

Here I am thinking of the browser case as pure-ESM, but not necessarily a browser as such. So may be running on command-line and ANSI is still relevant. I think a separate entry-point is appropriate rather than using "browser" condition in the conditional exports so it can be opted-into by clients from as-yet-unknown contexts. (This is not an independent idea, there is a browser entry point in yargs and yargs-parser, but I am working through the reasoning myself!)

Status quo

  • CommonJS: smart wrapping
  • ESM: dumb wrapping
  • Deno: dumb wrapping
  • browser: (use ESM)

Move to import map for dual versions of ANSI libraries?

As per #139. This gives smart wrapping everywhere, but turns out to break in yarn 1. So rejecting on that count.

Use appropriate source for ANSI library based on context?

  • CommonJS: use CommonJS ANSI libraries for smart wrapping
  • ESM: use CommonJS ANSI libraries for smart wrapping
  • Deno: use ESM ANSI libraries via URL for smart wrapping
  • browser: use ESM ANSI libraries via URL for smart wrapping

This means we stay on an older version of the ANSI libraries for node clients, but only need one copy of the libraries in our package.json dependencies.

Deno and browser get the ESM ANSI support libraries using an URL. Everybody gets smart wrapping.

Vendor CommonJS ANSI libraries?

  • CommonJS: vendor CommonJS ANSI libraries for smart wrapping
  • ESM: use ESM ANSI libraries for smart wrapping
  • Deno: use ESM ANSI libraries via URL for smart wrapping
  • browser: use ESM ANSI libraries via URL for smart wrapping

Functionally fine, but I don't really want to vendor libraries, and don't want to ship two copies.

@shadowspawn
Copy link
Member

I currently like "Use appropriate source for ANSI library based on context". I'm planning to give that a go soon, unless someone points out shortcomings first!

@shadowspawn
Copy link
Member

The area my knowledge is weak is bundlers. Do they generally either have support for hybrid CommonJS and ESM implementations, or support for imports from URLs?

@isaacs
Copy link

isaacs commented May 15, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants