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.

Sign up for GitHub

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

Jump to bottom

Version 4.0.0 #351

Closed
ellbee opened this issue Jun 22, 2018 · 11 comments
Closed

Version 4.0.0 #351

ellbee opened this issue Jun 22, 2018 · 11 comments

Comments

@ellbee
Copy link
Collaborator

ellbee commented Jun 22, 2018 •
edited
Loading

As we have some changes to the TypeScript typings and I am nervous to release them in a minor release (as they have broken peoples builds in the past), I am planning to release version 4.

Currently planned features:

Help wanted:
This was referenced Jun 22, 2018
Merged
Closed
@wmertens
Copy link

How about adding a createNonPureSelector? #231 (comment)

@MaxZhuravsky
Copy link

Maybe add createAsyncSelector #142 ?

@skortchmark9
Copy link

Any progress on releasing this? I'm pretty excited

@ellbee
Copy link
Collaborator Author

ellbee commented Sep 7, 2018

I have released 4.0.0-beta.1 to npm which addresses:

@skortchmark9
Copy link

Ah yeah, I noticed that - but it was 2 months ago! What are the conditions for releasing the actual 4.0.0?

@ellbee
Copy link
Collaborator Author

ellbee commented Sep 21, 2018

Let's have one last go at solving #340 and if we don't come up with anything satisfactory in the next week or so go ahead and release. How do people feel about the suggestion in #366 to only have the typings cover the array of input selectors?

@markerikson
Copy link
Contributor

@ellbee : my big concern with switching the order of input vs output selectors is that it's a huge breaking change for Reselect. I'm not entirely against it, but there's going to be a lot of people out there who do yarn add reselect and suddenly it doesn't work the way they expect.

@ellbee
Copy link
Collaborator Author

ellbee commented Sep 21, 2018

Yes, I 100% agree that switching the order will cause a lot of disruption. I was referring to the last paragraph, the suggestion of only having the typings cover the case where the input selectors are given in an array as the first parameter. If you are not using Typescript you shouldn't be affected at all.

@jimbol
Copy link
Contributor

jimbol commented Sep 25, 2018

Ready for the update :)

@ellbee
Copy link
Collaborator Author

ellbee commented Sep 30, 2018

Released!

If a solution for #340 is agreed upon we'll release it in a version 5.

@AlexeiDarmin AlexeiDarmin mentioned this issue Oct 18, 2018
Closed
@nickserv
Copy link
Contributor

Reselect 4.0.0 is out, it seems like this can be closed.

@MajorLift MajorLift mentioned this issue Dec 11, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this issue Dec 13, 2024
@MajorLift @metamaskbot
chore: Bump reselect to ^5.1.1 for heterogenously-typed selectors…
c671bf3 
… 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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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 issue Dec 18, 2024
@MajorLift @metamaskbot @danjm
chore: Bump reselect to ^5.1.1 for heterogenously-typed selectors…
6cc3672 
… 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

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@wmertens @nickserv @markerikson @jimbol @ellbee @skortchmark9 @MaxZhuravsky