preactjs / preact-router

:earth_americas: URL router for Preact.
http://npm.im/preact-router
MIT License
1.01k stars 156 forks source link

Fix exports map to support loading in Node esm #425

Closed fgnass closed 2 years ago

fgnass commented 2 years ago

Currently, preact-router fails to load in Node.js when it is imported from an ES module. Node correctly picks up the .module.js version as specified in the exports map, but loads it as cjs module because of the file extension. Here is a simple StackBlitz example that shows the current behavior.

This PR fixes the problem by changing the file extension to .mjs.

fgnass commented 2 years ago

Okay, you mean there are Node.js versions that support the "type" field but no exports maps? Or do you refer to bundlers that might also have adapted Node's file extension scheme but neither support the "module" field nor export maps? I had naively assumed, that all tools that know how to interprete the "type" field also support at least one way to specify CommonJS fallbacks. But I totally agree to play it safe here 👍

Ryan Christian @.***> schrieb am Do., 26. Mai 2022, 00:07:

@.**** commented on this pull request.

In package.json https://github.com/preactjs/preact-router/pull/425#discussion_r882152799 :

  • "module": "dist/preact-router.module.js",
  • "jsnext:main": "dist/preact-router.module.js",
  • "module": "dist/preact-router.mjs",
  • "jsnext:main": "dist/preact-router.mjs",

"type": "module" would necessitate that all CJS entries switch to .cjs, which has even less support.

.mjs got some popularity and use a few years before .cjs & the current Node extension semantics were specc'd and solidified.

— Reply to this email directly, view it on GitHub https://github.com/preactjs/preact-router/pull/425#discussion_r882152799, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAV22XA5W4YAUAMR3SOEKTVL2QCJANCNFSM5W2ADQ2A . You are receiving this because you authored the thread.Message ID: @.***>

rschristian commented 2 years ago

There's a few odd Node versions that have some issues around "type" and export maps, but they're not really the issue.

The issue is that bundlers have consumed "module" and "main" for many years now, long before Node's .[cm]js semantics have existed. They may throw at seeing either of those extensions, as they predate them existing. react-native's Metro bundler is an example of such a tool, and just last week caused an issue when immer released a change containing "module": "<path>.mjs". Turns out Metro still has 0 support for .mjs: https://github.com/immerjs/immer/issues/937

So it's best to keep those newer extensions locked behind "exports", as everything that can consume exports can handle those extensions just fine (or it's a bug if they cannot). For legacy fields, we have no guarantees that they'll support those extensions so we should try to keep them as .js to avoid breaking changes.

In this case, we add .mjs to "exports", keep all other ESM outputs as .js as Node's semantics do not need to apply to fields Node will never consume. Older versions of Node (out-of-date, mind you) will not consume "exports" and could throw at seeing .cjs for "main", so that too should be avoided (hence why "type": "module" would be bad).

fgnass commented 2 years ago

I updated the branch so that only the "module" exports use the .mjs extension. I used the copyfiles devDependency as it was already in place.

rschristian commented 2 years ago

Silly issue; somehow the test suite picks up and uses the new preact-router.mjs over the CJS version, even in a require() call. Just needed to specify the extension.

rschristian commented 2 years ago

Thanks for putting this together!

fgnass commented 2 years ago

It was a pleasure. Are there any plans for a new release on npm?

lkmill commented 2 years ago

@rschristian would be really nice if this could be published to npm... i currently cant upgrade my projects to "true" esm due to this.,..

rschristian commented 2 years ago

@lohfu Would probably recommend preact-iso over this anyways which does offer ESM, but I'll see if we can get a publish here.

rschristian commented 2 years ago

Jovi released v4.1.0 earlier today which includes this PR. Hope it helps!

lkmill commented 2 years ago

@rschristian cheers!