fix: Fix importing ES module from Node.js (#901) #921

cdauth · 2022-04-04T12:33:24Z

This pull request fixes #901 by publishing the ES module with the .mjs file extension.

For now, the existing ESM bundle dist/immer.esm.js is simply copied to dist/immer.esm.mjs building. This makes sure that there are no breaking changes, just in case any library includes the old file manually. In the next major release of Immer, the old bundle can be removed. By then jaredpalmer/tsdx#1059 might be released, which I believe will adapt the bundle file extensions.

This pull request also adds an exports field to package.json, which will make sure that Node.js will import the ES module when run in ESM mode. The wildcard export "./*": "./*" will make sure that any sub paths can be continued to be imported (for example something like import 'immer/src/immer.ts', which makes this a non-breaking change.

netlify · 2022-04-04T12:34:43Z

✅ Deploy Preview for quizzical-lovelace-dcbd6a canceled.

Name	Link
🔨 Latest commit	`afaa9e7`
🔍 Latest deploy log	https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/624ae59599369000082bf4df

mweststrate · 2022-04-04T14:50:21Z

package.json

+      "import": "./dist/immer.esm.mjs",
+      "require": "./dist/index.js"
+    },
+    "./*": "./*"


I don't think this one is needed; there is nothing else to import in immers case.

mweststrate · 2022-04-04T14:50:59Z

Looking good! Would you mind removing the * thing? I don't think there is any case where that is needed :)

cdauth · 2022-04-04T14:57:52Z

While the official usage recommendation of Immer might not include any imports of sub paths, I could imagine that there are some projects that use Immer by importing from immer/src/immer.ts or by importing from immer/dist/immer.esm.js for example. Because these imports would not work anymore without the wildcard export, introducing an export map usually seems to be considered a breaking change and thus requires a major release.

If you say that importing sub paths is not a supported use case and thus breaking it is not considered a breaking change, or if the next release will be a major release anyways and thus a breaking change is acceptable, I can remove the wild-card export. In that case, I would also remove the old dist/immer.esm.js bundle rather than keeping both the old and the new bundle, since with the export map the old bundle cannot be reached anymore anyways.

mweststrate · 2022-04-04T15:14:37Z

The import *.ts stuff is not a use case, since that typically doesn't work with build tools (it is only useful for source maps). And indeed breaking incorrect usage is not a breaking change imho :). But I didn't consider the following consequence. So let's keep things as is:

since with the export map the old bundle cannot be reached anymore anyways.

coveralls · 2022-04-04T15:26:01Z

Pull Request Test Coverage Report for Build 2089859747

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 98.553%

Totals
Change from base Build 1969317630:	0.0%
Covered Lines:	786
Relevant Lines:	794

💛 - Coveralls

mweststrate · 2022-04-04T15:34:45Z

Merging as is. Typically whenever I touch something ESM related, the skies come tumbling down, so brace yourselves. Should be available as 9.1.0 in ~30 minutes

cdauth · 2022-04-04T15:47:58Z

Nice! I'm also not 100% sure whether nothing will break, since there are so many module bundlers that handle modules in a different way. I'm curious to find out whether any reports will come in.

github-actions · 2022-05-11T17:44:53Z

🎉 This PR is included in version 9.0.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

MoreSaltMoreLemon · 2022-05-11T20:21:06Z

@cdauth

Nice! I'm also not 100% sure whether nothing will break, since there are so many module bundlers that handle modules in a different way. I'm curious to find out whether any reports will come in.

Hello!

One such breakage is coming from Redux Tookit, which has an unpinned usage of immer

v9.0.13 includes this PR, and breaks with React-Native's Metro module resolution for this package:

error: Error: While trying to resolve module `immer` from file `/Users/<user>/<app>/mobileApp/node_modules/@reduxjs/toolkit/dist/redux-toolkit.cjs.production.min.js`, the package `/Users/<user>/<app>/mobileApp/node_modules/immer/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/<user>/<app>/mobileApp/node_modules/immer/dist/immer.esm.mjs`. Indeed, none of these files exist:

  * /Users/<user>/<app>/mobileApp/node_modules/immer/dist/immer.esm.mjs(.native|.ios.js|.native.js|.js|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
  * /Users/<user>/<app>/mobileApp/node_modules/immer/dist/immer.esm.mjs/index(.native|.ios.js|.native.js|.js|.ios.json|.native.json|.json|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
    at DependencyGraph.resolveDependency (/Users/<user>/<app>/mobileApp/node_modules/metro/src/node-haste/DependencyGraph.js:376:17)
    at Object.resolve (/Users/<user>/<app>/mobileApp/node_modules/metro/src/lib/transformHelpers.js:271:42)
    at resolve (/Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:571:33)
    at dependencies.reduce (/Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:587:26)
    at Array.reduce (<anonymous>)
    at resolveDependencies (/Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:586:33)
    at /Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:275:33
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (/Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:87:24)
    at _next (/Users/<user>/<app>/mobileApp/node_modules/metro/src/DeltaBundler/traverseDependencies.js:107:9)

Anyone running into this: use npx npm-force-resolutions and pin the dependency to 9.0.12 instead.

cdauth · 2022-05-11T22:31:37Z

@MoreSaltMoreLemon I haven't tried the new version yet, but the fact that I was having trouble with redux-toolkit was the original reason why I opened this pull request. I have also opened one with redux-toolkit (reduxjs/redux-toolkit#2200), but it has not been merged yet.

The error you are seeing looks strange to me, since the file immer.esm.mjs should actually exist. This seems to be a bug in metro: facebook/metro#535. Some workarounds are described there. I would expect more and more packages to use the .mjs/.cjs extensions, so I would say this problem needs to be solved in metro.

MoreSaltMoreLemon · 2022-05-11T23:18:51Z

@cdauth

The error you are seeing looks strange to me, since the file immer.esm.mjs should actually exist. This seems to be a bug in metro: facebook/metro#535. Some workarounds are described there. I would expect more and more packages to use the .mjs/.cjs extensions, so I would say this problem needs to be solved in metro.

I continued looking into it after my response and tried to recreate in a fresh create-react-native-app, and couldn't. Also couldn't recreate in a regular react-app.

It appears that the issue is simply our using an older version of metro which predates .mjs support. Even though the file definitely exists, it isn't able to read it, and consequently tries to append a file extension that it can read to the provided filename and fails.

So resolutions are temporarily lock the version to 9.0.12 and update react-native/metro to a newer version with .mjs support.

rschristian · 2022-05-12T02:59:15Z

Beyond the fields that Node consumes (and therefore requires the (cm)js semantics), it's generally better to keep .js for back compat.

It's only going to be an issue in older build tools, but regardless, this change was a semver major. "module", "jsnext:main", and "react-native" should be rolled back.

francois-codes · 2022-05-12T08:58:06Z

So things did break, for immer users but also users of libraries relying on immer (reduxjs/toolkit for one)

Shouldn't this be reverted and included in a major change instead ?

fix: Fix importing ES module from Node.js (#901)

afaa9e7

mweststrate reviewed Apr 4, 2022

View reviewed changes

mweststrate merged commit b2db62b into immerjs:master Apr 4, 2022

github-actions bot added the released label May 11, 2022

markerikson mentioned this pull request May 11, 2022

Issues with immer version v9.0.13 reduxjs/redux-toolkit#2315

Closed

markerikson mentioned this pull request May 11, 2022

Possible breaking change in package exports in v9.0.13 #937

Closed

1 task

cdauth mentioned this pull request May 12, 2022

fix: Use .esm.js module for backwards compatibility with old build tools #939

Merged

mweststrate mentioned this pull request Jan 20, 2023

Immer 10 proposals: Drop promises, default export, array.length patches, ES5 mode, and make getter / setter support opt-in #1015

Closed

13 tasks

andreialecu mentioned this pull request Feb 5, 2024

Add exports to package.json apollographql/apollo-client#11570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix importing ES module from Node.js (#901) #921

fix: Fix importing ES module from Node.js (#901) #921

cdauth commented Apr 4, 2022

netlify bot commented Apr 4, 2022 •

edited

Loading

mweststrate Apr 4, 2022

mweststrate commented Apr 4, 2022

cdauth commented Apr 4, 2022

mweststrate commented Apr 4, 2022

coveralls commented Apr 4, 2022 •

edited

Loading

mweststrate commented Apr 4, 2022

cdauth commented Apr 4, 2022

github-actions bot commented May 11, 2022

MoreSaltMoreLemon commented May 11, 2022 •

edited

Loading

cdauth commented May 11, 2022

MoreSaltMoreLemon commented May 11, 2022 •

edited

Loading

rschristian commented May 12, 2022

francois-codes commented May 12, 2022

fix: Fix importing ES module from Node.js (#901) #921

fix: Fix importing ES module from Node.js (#901) #921

Conversation

cdauth commented Apr 4, 2022

netlify bot commented Apr 4, 2022 • edited Loading

✅ Deploy Preview for quizzical-lovelace-dcbd6a canceled.

mweststrate Apr 4, 2022

Choose a reason for hiding this comment

mweststrate commented Apr 4, 2022

cdauth commented Apr 4, 2022

mweststrate commented Apr 4, 2022

coveralls commented Apr 4, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2089859747

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mweststrate commented Apr 4, 2022

cdauth commented Apr 4, 2022

github-actions bot commented May 11, 2022

MoreSaltMoreLemon commented May 11, 2022 • edited Loading

cdauth commented May 11, 2022

MoreSaltMoreLemon commented May 11, 2022 • edited Loading

rschristian commented May 12, 2022

francois-codes commented May 12, 2022

netlify bot commented Apr 4, 2022 •

edited

Loading

coveralls commented Apr 4, 2022 •

edited

Loading

MoreSaltMoreLemon commented May 11, 2022 •

edited

Loading

MoreSaltMoreLemon commented May 11, 2022 •

edited

Loading