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

feat: add proper wrapping for esm and Deno by updating dependencies #143

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

shadowspawn
Copy link
Member

@shadowspawn shadowspawn commented May 24, 2023

This is yet another approach for adding wrapping for ESM and Deno, see also #119 #139 #140 !

Problem

Only CJS currently gets smart wrapping (#89 #113 #138 yargs/yargs#2112)

Solution

Upgrade to newer ansi dependencies which are ESM and use rollup to transpile the CJS version.

  • the tests are refactored so that full tests run for both CJS and ESM
  • add ESM test to npm run-script for test, run both cjs and esm tests together

Notes for interest of reviewers:

  • The ansi dependencies for Deno are specified implicitly by package.json. I like package.json so have a single source of truth for dependency versions, but I don't know the tradeoffs vs using an URL to a CDN, and if it works ok when published to deno.land.
  • Two of the transient dependencies are cjs, so the esm build is no longer "pure" esm.
  • Not updating to rollup@3 as it requires node 14. (And cliui still only requires node 12.)
  • Not updating to string-width@6 as it requires requires node 16.

@shadowspawn

This comment was marked as outdated.

@shadowspawn shadowspawn marked this pull request as ready for review May 25, 2023 07:09
- add esm test to npm run script
- run full tests from esm

Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
@bcoe
Copy link
Member

bcoe commented Nov 21, 2023

Feeling inspired to do a little bit of open source again, apologies for radio silence.

Any preference between this implementation and @isaacs #139

@isaacs
Copy link

isaacs commented Nov 21, 2023

I'm happy to switch back from my fork once it has the same behavior on esm as cjs, whether it's the pr I sent or this one or some other one.

@shadowspawn
Copy link
Member Author

shadowspawn commented Nov 22, 2023

I was concerned the approach with aliases in #139 and used in jackspeak triggered multiple problem reports from users of yarn. We are using rollup though various yargs repos to help with the hybrid cjs/esm, so already have a tool for the job.

Reports of problems with yarn 1:

And one problem with yarn 3:

@shadowspawn

This comment was marked as outdated.

@shadowspawn
Copy link
Member Author

There was a clash between versions of TypeScript from 4.7 and rollup@2 that was fixed in rollup@3.0.0, but that rollup requires node 14. Pinning back TypeScript to 4.6.4 works around the problem.

(Now, what is going wrong with that coverage check...)

@shadowspawn

This comment was marked as outdated.

@isaacs
Copy link

isaacs commented Nov 23, 2023

There is absolutely no reason for any module to keep supporting node 14, let alone node 12. (For that matter, I obviously don't see the point in trying to support yarn 1, but here we are.)

If someone is using EOL versions of node, they are already off the supported landscape, and can be expected to either very carefully lock down everything, or be broken repeatedly.

@shadowspawn
Copy link
Member Author

There is absolutely no reason for any module to keep supporting node 14, let alone node 12

Yes, I am just trying to work with the current constraints so this PR is independent of a bumping minimum node to (say) 18. (And freshly reminded of what a pain that can be.)

@bjester
Copy link

bjester commented Feb 22, 2024

Just wanted to say that I appreciate the effort put into this, as it helps immensely in transitioning fully to ESM. The fork of this package used in glob has only caused us (learning equality) issues, specifically as time hasn't been put into upgrading yarn (yet).

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 this pull request may close these issues.

4 participants