-
Notifications
You must be signed in to change notification settings - Fork 21
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
📦 Patch Redux + use RTK alpha for full ESM support #37
Conversation
b64d7b6
to
003349e
Compare
/snapit |
🫰✨ Thanks @QuintonC! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/[email protected] yarn add @shopify/[email protected] yarn add @shopify/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We paired on this and it seems fine, but how will we maintain this? Does it require documentation? Should there be some issue to look into this again a couple months from now?
Also, the codesandbox links open and I understand they are for next, but should we have some place to tophat our regular installation (non-next)?
Edit: After thinking about this, I figured I can just clone and test as usual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tophatted on metamask desktop 👍
Plus one on this. Do we know if it is going to get patched to the actual package in the future? |
@jamiely @caropinzonsilva great questions. From my understanding, this is actively being worked on by the folks over at Redux. The patch we're using here should be resolved when this PR is merged. If that PR doesn't get merged there's not any concern either as it's on the Redux team's active roadmap to remove the |
Sound good! Can we create an issue for it? So that we don't totally forget about it :P |
Yeah, of course! I'll get one created momentarily 🙌 Issue created! #46 |
003349e
to
dfa32ad
Compare
ℹ️ What is the context for these changes?
Addresses an issue with Redux Toolkit and ESM support. When attempting to use the package in Next.js an error would be present stating addListener was not exported. This was primarily due to the lack of support for ESM usage in Redux Toolkit, which version 2 aims to resolve.
This makes use of an alpha package and a patch in order to remove an
EmptyObject
type from the ReduxCombinedState
type.After chatting with some of the team from Redux, it appears the only "downside" to removing
EmptyObject
from theCombinedState
type is that we must ensure that ourinitialState
passed tocombineReducers
has all keys defined (which we already did anyway).🕹️ Demonstration
🎩 How can this be tophatted?
There are demo links on the demonstration :)
✅ Checklist
Tested on mobileN/ATested on multiple browsersN/ATested for accessibilityN/AIncludes unit testsN/AUpdated relevant documentation for the changes (if necessary)N/A