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

perf: blazingly fast #7799

Merged
merged 11 commits into from
Sep 25, 2024
Merged

perf: blazingly fast #7799

merged 11 commits into from
Sep 25, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Sep 24, 2024

Description

Attempt 1 at redux perf. improvements

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

High - touches state. Getting this wrong could brick state, and we'd have to revert this and do a nuke of the whole state.

Testing

  • Ensure app state still looks good
  • Ensure rehydration (i.e refresh with persisted state) is still happy

Engineering

  • ^
  • Migrations are still happy
    NOTE: Ensure you check for slice state after rehydration (i.e {rehydrated: true} for the slice/s you're migrating) to ensure redux-persist rehydrated with your migration:
    Screenshot 2024-09-24 at 22 27 36
    Screenshot 2024-09-24 at 22 27 26

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

https://jam.dev/c/6fee8b7a-97d8-48b0-b420-b04f3ef008b4

@gomesalexandre gomesalexandre changed the title perf: split persistReducers per slice perf: blazingly fast Sep 24, 2024
@gomesalexandre gomesalexandre marked this pull request as ready for review September 24, 2024 20:34
@gomesalexandre gomesalexandre requested a review from a team as a code owner September 24, 2024 20:34
Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise very sane, will do second pass of runtime testing before giving final stamp

src/state/migrations/clearOpportunities.ts Show resolved Hide resolved
src/state/migrations/index.ts Show resolved Hide resolved
src/state/store.ts Show resolved Hide resolved
src/state/migrations/index.ts Show resolved Hide resolved
src/state/reducer.ts Show resolved Hide resolved
src/state/reducer.ts Show resolved Hide resolved
Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested a couple of migrations and they worked as expected / properly rehydrated!

Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Regeneration assets is properly creating a new line inside clearAssetsMigrations
  • Assets migration seems to be working as expected
  • Rehydration also seems to work as expected

I had a problem with the history of BTC but as seen in pm, it doesn't look linked with this PR

We have to take a lot of care with this PR because it will be pretty hard to revert if we push it to production, people would end in a borked state (I did try to come back to develop and I had to flush my cache to make it work)

@gomesalexandre gomesalexandre enabled auto-merge (squash) September 25, 2024 21:54
@gomesalexandre gomesalexandre merged commit 0f84983 into develop Sep 25, 2024
6 checks passed
@gomesalexandre gomesalexandre deleted the feat_split_reducers_persist branch September 25, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants