-
Notifications
You must be signed in to change notification settings - Fork 22
📦 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/connect-wallet@0.0.0-snapshot-20230302224907yarn add @shopify/gate-context-client@0.0.0-snapshot-20230302224907yarn add @shopify/tokengate@0.0.0-snapshot-20230302224907 |
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
EmptyObjecttype from the ReduxCombinedStatetype.After chatting with some of the team from Redux, it appears the only "downside" to removing
EmptyObjectfrom theCombinedStatetype is that we must ensure that ourinitialStatepassed tocombineReducershas 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