-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
feat: redux migration #6830
feat: redux migration #6830
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: redux-localstorage-simple@2.4.0 |
1 flaky tests on run #13732 ↗︎
Details:
cypress/e2e/token-explore-filter.test.ts • 1 flaky test • e2e
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|
||
persistStore(store) | ||
// wait for the migration to complete | ||
await new Promise((resolve) => setTimeout(resolve, 0)) |
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.
they don't give us an await persistStore i guess
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.
overall looks good
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.
looks clean and I like the tests :)
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.
I think you need a PersistGate
(see https://www.npmjs.com/package/redux-persist) or something similar to make sure the state is loaded before the first render.
You may not want a PersistGate
, to avoid a slow pageload experience, but it doesn't seem to pick up my previously connected wallet when I reload the page.
Can you also add an e2e test that ensures that saved state is being reflected in pageload? |
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.
awesome
cypress/support/commands.ts
Outdated
...(options?.userState ?? {}), | ||
}, | ||
}, | ||
'persist:interface' |
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.
This should be pulled out as a key from ../../src/state/user/reducer
if it can be.
abce139
to
91ec8f3
Compare
Description
introduces a version management system for the redux state - tests for the type of the App State, migrations which run when versions don't match, and an initial migration to move away from the old library.
Linear ticket: https://linear.app/uniswap/issue/WEB-2224/add-redux-migrations-to-the-interface
Relevant docs: https://www.notion.so/Redux-migrations-in-interface-30aca899626a4ac4af7b070f92ce976d
Screen capture
Before Migration
After Migration (updated 8/14 to show IndexedDB)
Test plan
QA (ie manual testing)
redux-localstorage-simple
and ensured my old state was copied overAutomated testing