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

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

Closed
3 tasks done
JoshuaKGoldberg opened this issue Nov 20, 2023 · 4 comments · Fixed by #154 or #157
Closed
3 tasks done

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

JoshuaKGoldberg opened this issue Nov 20, 2023 · 4 comments · Fixed by #154 or #157
Labels
area: tooling Managing the repository's maintenance status: accepting prs Please, send a pull request to resolve this!

Comments

@JoshuaKGoldberg
Copy link
Collaborator

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Overview

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

node-emoji/package.json

Lines 30 to 35 in 6714e21

"exports": {
".": {
"import": "./lib/index.js",
"require": "./lib/index.cjs"
}
},

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
Copy link

BatuhanW commented Nov 20, 2023

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.

refinedev/refine#5279 (comment)

@JoshuaKGoldberg
Copy link
Collaborator Author

JoshuaKGoldberg commented Nov 20, 2023

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...
> [email protected] 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
Copy link
Collaborator Author

#157 pins this package's skin-tone dependency down to 2.0.0. [email protected] 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 happy1 to take a look. Thanks all!

Footnotes

  1. As happy as I can be investigating something I broke, that is...

@BatuhanW
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tooling Managing the repository's maintenance status: accepting prs Please, send a pull request to resolve this!
Projects
None yet
2 participants