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

refactor: Unflatten selectors and convert to TypeScript #29014

Draft
wants to merge 13 commits into
base: jongsun/perf/redux/241204-unflatten-metamask-slice
Choose a base branch
from

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Dec 9, 2024

Motivation

Converting selectors to TypeScript is strictly speaking out-of-scope for this epic, but I decided to undertake it for these reasons:

  • Enables formal validation of the "unflattening" refactors, which are too extensive for completely reliable verification by human reviewers.
  • Large degree of overlap in workload.
  • Typing blockers resolved now that we can take full advantage of heterogeneously-typed selector composition.

Description

  • TypeScript conversions for files that contain selectors which directly or indirectly (i.e. via nested selectors) reference the metamask slice.
  • Wherever practical, redefine selectors that take a state parameter to use create{,DeepEqual}Selector instead.
    • Replace explicit state type assignment with type dynamically derived from selector composition.
  • Type fixes e.g. consistent typing for parameters passed through multiple levels and branches of nested selectors.

Notes

  • The motivating changes are located in ui/selectors, ui/ducks, ui/helpers/utils, shared/modules/selectors.
  • This commit is a good starting point for review: 9c931d8?w=1#diff-fc57d736577d4f0d4904397b8f14d9294198122f9805eba704af344bed525f2a
    • These diffs are not visible in the "Files changed" tab, because GitHub refuses to recognize these TypeScript conversions as renames.
  • (investigating) This PR increases the circular dependencies count in ui/selectors (excluding type imports) from 5 to 33.
Screenshot 2025-01-07 at 12 19 09 PM Screenshot 2025-01-07 at 12 20 28 PM

Open in GitHub Codespaces

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Dec 9, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@MajorLift MajorLift added team-tiger Tiger team (for tech debt reduction + performance improvements) and removed team-wallet-framework labels Dec 9, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from d47b4eb to 143f1a2 Compare December 9, 2024 15:43
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from ba499fd to 7ad7c9a Compare December 9, 2024 15:46
@MajorLift MajorLift changed the title perf: Unflatten metamask slice selectors and convert to TypeScript perf: Unflatten metamask slice selectors and convert to TypeScript (3/5) Dec 9, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 7ad7c9a to e17a049 Compare December 9, 2024 18:55
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch 4 times, most recently from 3880292 to 66188c8 Compare December 10, 2024 12:10
@MajorLift MajorLift changed the title perf: Unflatten metamask slice selectors and convert to TypeScript (3/5) perf: Unflatten selectors and convert to TypeScript (3/5) Dec 10, 2024
@MajorLift MajorLift changed the title perf: Unflatten selectors and convert to TypeScript (3/5) perf: Convert all selectors to TypeScript and unflatten references to metamask slice (3/5) Dec 10, 2024
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from e17a049 to 168e2b7 Compare December 10, 2024 14:37
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 66188c8 to dd75333 Compare December 10, 2024 14:41
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 2 times, most recently from 0971827 to c557fee Compare December 10, 2024 15:15
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch 2 times, most recently from f4ebdb6 to 63d8618 Compare December 10, 2024 15:35
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from c557fee to 8384a85 Compare December 10, 2024 15:35
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 4419d3e to f8ca26b Compare December 10, 2024 16:35
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 8384a85 to 44dde0e Compare December 10, 2024 18:01
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from f8ca26b to ebdd010 Compare December 10, 2024 18:01
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 44dde0e to cf9e3ae Compare December 10, 2024 18:15
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch 4 times, most recently from 14b4b24 to 062dcc8 Compare December 10, 2024 20:16
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from cf9e3ae to f854428 Compare December 11, 2024 09:35
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from d062d2c to f629232 Compare January 6, 2025 12:51
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch from 62d01dd to b1ce481 Compare January 6, 2025 16:18
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch 2 times, most recently from 00b71d0 to 8f18a7a Compare January 6, 2025 18:13
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241204-unflatten-metamask-slice branch 2 times, most recently from 1add77c to c47ed91 Compare January 6, 2025 19:39
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 13db6b8 to 689dc40 Compare January 6, 2025 20:05
@MajorLift MajorLift added DO-NOT-MERGE Pull requests that should not be merged team-extension-platform labels Jan 6, 2025
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 3d79a33 to 89263ea Compare January 7, 2025 12:14
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from d656eaa to 7cab4c4 Compare January 7, 2025 17:39
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 7cab4c4 to 55fc96e Compare January 7, 2025 17:45
@MajorLift MajorLift changed the title refactor: Convert selectors to TypeScript and unflatten references to metamask slice (3/5) refactor: Convert selectors to TypeScript and unflatten references to metamask slice (3/6) Jan 7, 2025
@MajorLift MajorLift changed the title refactor: Convert selectors to TypeScript and unflatten references to metamask slice (3/6) refactor: Unflatten selectors and convert to TypeScript (3/6) Jan 7, 2025
Force github to recognize renames part 2

Force github to recognize renames part 3
@MajorLift MajorLift force-pushed the jongsun/perf/redux/241206-convert-selectors-to-typescript branch from 1852790 to 9c931d8 Compare January 7, 2025 18:11
@MajorLift MajorLift changed the title refactor: Unflatten selectors and convert to TypeScript (3/6) refactor: Unflatten selectors and convert to TypeScript Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged team-extension-platform team-tiger Tiger team (for tech debt reduction + performance improvements)
Projects
Status: Needs more work from the author
Development

Successfully merging this pull request may close these issues.

2 participants