omnidan / node-emoji

💖 simple emoji support for node.js projects
MIT License
1.31k stars 241 forks source link

🛠 Tooling: Add test that CJS require() works (not just ESM import) #153

Closed JoshuaKGoldberg closed 10 months ago

JoshuaKGoldberg commented 10 months ago

Bug Report Checklist

Overview

This library is meant to support both CommonJS (CJS / require()) and ECMAScript Modules (ESM / import) entry points:

https://github.com/omnidan/node-emoji/blob/6714e2102acf0cf6a556f168952f90582b1a1663/package.json#L30-L35

But, per #151, it's possible for a change to make it in that breaks just CJS. We should add in some kind of end-to-end/integration test to make sure that CJS isn't broken.

Additional Info

No response

BatuhanW commented 10 months ago

Hello 👋🏼

We are affected by release version 2.1.1, locked our dependency to 2.1.0, but since marked-terminal also uses ^2.1.0, we still have problems. It works with npm, but with yarn and pnpm package managers, it still doesn't work.

Wanted to inform that we got report using 2.1.2 version, as you can see in the screenshot, I think this issue still exists in version 2.1.2.

https://github.com/refinedev/refine/issues/5279#issuecomment-1819429005

JoshuaKGoldberg commented 10 months ago

Hey @BatuhanW! First of all, sorry for breaking your (and @askoufis's) builds. That was a miss on my part.

A workaround that should work across package managers in the case of a dependency breaking, including a deep dependency-of-a-dependency, is using an overrides/resolutions style pin.

I can reproduce your issue with npm:

  1. Go through the quickstart in https://refine.dev/docs/getting-started/quickstart/, selecting npm, Next.js, and nothing else
  2. npm i and npm run dev: no issue
  3. Add "overrides": { "node-emoji": "2.1.2" to package.json
  4. npm i and npm run dev: the following...
> little-pianos-report@0.1.0 dev
> cross-env NODE_OPTIONS=--max_old_space_size=4096 refine dev

/Users/josh/repos/little-pianos-report/node_modules/node-emoji/lib/index.cjs:181
var import_skin_tone = __toESM(require("skin-tone"), 1);
                               ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/josh/repos/little-pianos-report/node_modules/skin-tone/index.js from /Users/josh/repos/little-pianos-report/node_modules/node-emoji/lib/index.cjs not supported.
Instead change the require of index.js in /Users/josh/repos/little-pianos-report/node_modules/node-emoji/lib/index.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/josh/repos/little-pianos-report/node_modules/node-emoji/lib/index.cjs:181:32)
    at Object.<anonymous> (/Users/josh/repos/little-pianos-report/node_modules/marked-terminal/index.cjs:10:13)
    at Object.<anonymous> (/Users/josh/repos/little-pianos-report/node_modules/@refinedev/cli/dist/cli.js:25:1400) {
  code: 'ERR_REQUIRE_ESM'
}

Adding "skin-tone": "2.0.0" to package.json's "overrides" fixes it. I'll send a fix here momentarily reverting #152.

JoshuaKGoldberg commented 10 months ago

157 pins this package's skin-tone dependency down to 2.0.0. node-emoji@2.1.3 is published and has the fix.

If there are any other issues, please do file a new issue on this repo and I'd be happy[^1] to take a look. Thanks all!

[^1]: As happy as I can be investigating something I broke, that is...

BatuhanW commented 10 months ago

Hey @JoshuaKGoldberg it seems the version 2.1.3 works as expected, thanks for the quick resolution! Much appreciated 🙏🏼