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

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

Closed
1 task done
markerikson opened this issue May 11, 2022 · 10 comments · Fixed by nhost/nhost#640
Closed
1 task done

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

markerikson opened this issue May 11, 2022 · 10 comments · Fixed by nhost/nhost#640

Comments

@markerikson
Copy link
Contributor

🐛 Bug Report

We just had a user file a Redux Toolkit bug report saying that as of Immer v9.0.13, RTK fails to initialize correctly, with a classic Object(...) is not a function error message

reduxjs/redux-toolkit#2315

image

Looking at the changes in v9.0.12...v9.0.13 , I see that there was a recent PR changing how Immer does its exports:

#921

This seems like a likely candidate for causing this kind of problem.

Note that this change was not called out in the 9.0.13 release notes, and also seems like it's a much bigger change than a "patch"-level release. (FWIW I've been advised that changes to exports in package.json probably justify being breaking majors.)

Link to repro

Don't have a repro yet - the end user just reported this a few minutes ago.

Observed behavior

It looks like Immer's exports have potentially changed, depending on the build/run environment. (Don't know details on the end user's environment yet).

Expected behavior

Immer's imports load cleanly.

Environment

We only accept bug reports against the latest Immer version.

  • Immer version:
  • I filed this report against the latest version of Immer

Rest of user environment is unknown, have asked for more details.

@hirbod
Copy link

hirbod commented May 12, 2022

Your bundler most likely does not understand mjs. Had similar issues with React-Native and had to add mjs to sourceExts to make Metro happy

@markerikson
Copy link
Contributor Author

FWIW, it isn't "my" bundler :) I'm a Redux Toolkit maintainer (RTK relies on Immer), and one of our users reported the bug against RTK.

Tbh this is the sort of thing that scares me about trying to change package export setup for RTK itself, and why I refuse to touch our setup until we're ready for a major.

@hirbod
Copy link

hirbod commented May 12, 2022

So it's your users bundler :)
But I'm totally with you on that. I wouldn't just do something like that in a minor release either. This is actually a major change and should be rolled back.

And here a quote by one of the collaborators (@mweststrate)

Merging as is. Typically whenever I touch something ESM related, the skies come tumbling down, so brace yourselves.
#921 (comment)

Well, guess he has been right :)

@francois-codes
Copy link

related to #938

this is indeed a breaking change a big a problem for library maintainers.

We have a build system where we generate projects on the fly with a framework that has reduxjs/toolkit as a dependency (and therefore immer). All our builds on our existing versions of our framework are now broken because of this. This should definitely not be published as a patch update :/

I hope it will be rolled back ASAP and included in the next major version instead as @hirbod suggested

@HarshitMadhav
Copy link

HarshitMadhav commented May 12, 2022

@markerikson +1 I am a user of redux toolkit package and it is breaking on immer in react-native.

@francois-codes
Copy link

fixed in #939 and available in 9.0.14

@mweststrate
Copy link
Collaborator

Sorry for the inconvenience folks! Seems it is is somehow impossible to be a good ESM citizen without breaking stuff. At least no idea how to verify that it is done correctly before pushing it in the wild :).

@francois-codes
Copy link

no worries @mweststrate , you reacted promptly so that's good :)
Do you have a flow to release in npm's next channel ? that's how I would test this in real conditions before "pushing it in the wild"

@markerikson
Copy link
Contributor Author

@mweststrate thanks for the fix!

FWIW, my only real answer here is to do test publishes locally with yalc, and somehow have a battery of test projects set up using different tools, and see how the "published" package behaves.

@markerikson
Copy link
Contributor Author

Wanted to follow up on this in two ways:

  • I'm currently working on migrating Redux Toolkit 2.0 alpha to be a full ESM exports-based package (with CJS fallback). Current WIP PR is here: Migrate the RTK package to be full ESM reduxjs/redux-toolkit#3095 . I think I've got it mostly working as far as the package structure.
  • That said, where I'm running into trouble is actually with packages that export both a default value and named values, such as redux-thunk and Immer. (Interestingly, this seems to primarily be a problem with how Jest imports things.) I also maintain redux-thunk, so I can fix that one myself, but Immer is a separate issue.

It might be worth re-considering this set of changes for an Immer 10.0?

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 a pull request may close this issue.

5 participants