added typings for dynamic array of uniform selectors #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 similar comment
|
👍 |
|
Can anyone re-trigger the travis build? The first one failed with an issue not related to this change. What else this PR is missing to proceed? |
Codecov Report
@@ Coverage Diff @@
## master #315 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 15 15
=====================================
Hits 15 15Continue to review full report at Codecov.
|
DmShpak
approved these changes
Mar 14, 2018
|
Hey @sergey-su, sorry to be so late to this. I just merged another long-outstanding typings pr and seem to have introduced some merge conflicts with this pr. If you resolve them and we can find someone to review (I don't use typescript), then I'll get this merged too. |
sergey-su
force-pushed
the
dynamic-selectors-array
branch
from
ba61be7 to
36f256b
Compare
|
@ellbee , PR branch rebased |
|
I was hoping for someone who uses Typescript to approve it, but I see @aikoven approved it before the rebase so I'll merge. |
Closed
|
@ellbee Thanks! When are you planning a release that includes this fix? |
|
I'll likely release a beta sometime over the weekend. There is another TypeScript related issue that I'd like to see if we can clear up (#340) as part of v4 as I don't feel comfortable making TypeScript changes in minor versions. I don't use TypeScript personally so any help would be much appreciated! |
2 tasks
7 tasks
github-merge-queue bot
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Dec 13, 2024
… support (#29094) ## Motivation #### Homogenous selector types: ```ts const selectorOne = (state: State) => state.something; const selectorTwo = (state: State) => state.other; createSelector([selectorOne, selectorTwo], () => ...); ``` #### Heterogenous selector types: ```ts const selectorOne = (state: { something: string }) => state.something; const selectorTwo = (state: { other: number }) => state.other; createSelector([selectorOne, selectorTwo], () => ...); ``` Support for heterogenous typing is essential for selectors to function properly, because selectors must be both mergeable and atomic. Without heterogenous selectors, these becoming conflicting objectives. - **Mergeable:** - Without heterogenous typing for selectors, the size of the state type for all selectors tend to inflate. This is because a selector's state must be a supertype for the intersection of the state types of all of its merged selectors, including selectors nested in the definitions of merged selectors. - Eventually, many selectors end up being defined with a state type that is close to the entire Redux state. - Paradoxically, selectors that only need access to very few properties end up needing to have the widest state type, because they tend to be merged into the most selectors across several nested levels. - **Atomic:** - When selectors are actually invoked, including in test files, it's not always practical to prepare and pass in a very large state object. - It's both safer and more convenient to restrict the state being passed into the selector to the minimum size required for the selector to function. - This requirement becomes incompatible with the mergeability requirement if all selectors must share a common state type. Enabling merged selectors to accept different, even disjoint state types resolves this issue. > [!NOTE] > See the [`MultichainState`](https://github.com/MetaMask/metamask-extension/blob/4f970df0acec3e3bc80da08373aa3b16f23aae41/ui/selectors/multichain.ts#L53) type for an example of a bloated selector state type which will become unnecessary with this update. ## **Description** - `reselect@4.0.0` supports heterogenous typing for selector inputs to `createSelector` and `createDeepEqualSelector`. - reduxjs/reselect#351 - reduxjs/reselect#274 - reduxjs/reselect#315 - Upgrade to `^5.1.1` (latest) is necessary to fix type issues in `4.0.0` - https://app.circleci.com/jobs/github/MetaMask/metamask-extension/4320173 [](https://codespaces.new/MetaMask/metamask-extension/pull/29094?quickstart=1) ## **Related issues** - Blocks TypeScript conversion of selectors for MetaMask/MetaMask-planning#2894. - #29014 ## **Manual testing steps** ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
danjm
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Dec 18, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/29094?quickstart=1){kind=link}
… support (#29094) ## Motivation #### Homogenous selector types: ```ts const selectorOne = (state: State) => state.something; const selectorTwo = (state: State) => state.other; createSelector([selectorOne, selectorTwo], () => ...); ``` #### Heterogenous selector types: ```ts const selectorOne = (state: { something: string }) => state.something; const selectorTwo = (state: { other: number }) => state.other; createSelector([selectorOne, selectorTwo], () => ...); ``` Support for heterogenous typing is essential for selectors to function properly, because selectors must be both mergeable and atomic. Without heterogenous selectors, these becoming conflicting objectives. - **Mergeable:** - Without heterogenous typing for selectors, the size of the state type for all selectors tend to inflate. This is because a selector's state must be a supertype for the intersection of the state types of all of its merged selectors, including selectors nested in the definitions of merged selectors. - Eventually, many selectors end up being defined with a state type that is close to the entire Redux state. - Paradoxically, selectors that only need access to very few properties end up needing to have the widest state type, because they tend to be merged into the most selectors across several nested levels. - **Atomic:** - When selectors are actually invoked, including in test files, it's not always practical to prepare and pass in a very large state object. - It's both safer and more convenient to restrict the state being passed into the selector to the minimum size required for the selector to function. - This requirement becomes incompatible with the mergeability requirement if all selectors must share a common state type. Enabling merged selectors to accept different, even disjoint state types resolves this issue. > [!NOTE] > See the [`MultichainState`](https://github.com/MetaMask/metamask-extension/blob/4f970df0acec3e3bc80da08373aa3b16f23aae41/ui/selectors/multichain.ts#L53) type for an example of a bloated selector state type which will become unnecessary with this update. ## **Description** - `reselect@4.0.0` supports heterogenous typing for selector inputs to `createSelector` and `createDeepEqualSelector`. - reduxjs/reselect#351 - reduxjs/reselect#274 - reduxjs/reselect#315 - Upgrade to `^5.1.1` (latest) is necessary to fix type issues in `4.0.0` - https://app.circleci.com/jobs/github/MetaMask/metamask-extension/4320173 [](https://codespaces.new/MetaMask/metamask-extension/pull/29094?quickstart=1) ## **Related issues** - Blocks TypeScript conversion of selectors for MetaMask/MetaMask-planning#2894. - #29014 ## **Manual testing steps** ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently array version of
createSelectortyping has fixed number of selectors known at compile-time. HowevercreateSelector's implementation supports any number of selectors determined at run-time. Technically input selectors can have different return types. That's impossible to describe in TypeScript. But it's possible to describe the case when all selectors have same return type. For example when input selectors are generated for each element of a container that has nr of elements fixed during selector's lifetime: