Skip to content

Commit

Permalink
chore: Bump reselect to ^5.1.1 for heterogenously-typed selectors…
Browse files Browse the repository at this point in the history
… 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>
  • Loading branch information
2 people authored and danjm committed Dec 18, 2024
1 parent 84e4e6e commit 6cc3672
Show file tree
Hide file tree
Showing 13 changed files with 185 additions and 179 deletions.
16 changes: 8 additions & 8 deletions lavamoat/browserify/beta/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -1694,10 +1694,10 @@
"@metamask/network-controller>@metamask/eth-block-tracker": true,
"@metamask/network-controller>@metamask/eth-json-rpc-infura": true,
"@metamask/network-controller>@metamask/swappable-obj-proxy": true,
"@metamask/network-controller>reselect": true,
"@metamask/rpc-errors": true,
"@metamask/utils": true,
"eslint>fast-deep-equal": true,
"reselect": true,
"uri-js": true,
"uuid": true
}
Expand Down Expand Up @@ -1756,13 +1756,6 @@
"semver": true
}
},
"@metamask/network-controller>reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"@metamask/notification-controller>nanoid": {
"globals": {
"crypto.getRandomValues": true
Expand Down Expand Up @@ -5138,6 +5131,13 @@
"@babel/runtime": true
}
},
"reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"semver": {
"globals": {
"console.error": true
Expand Down
16 changes: 8 additions & 8 deletions lavamoat/browserify/flask/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -1694,10 +1694,10 @@
"@metamask/network-controller>@metamask/eth-block-tracker": true,
"@metamask/network-controller>@metamask/eth-json-rpc-infura": true,
"@metamask/network-controller>@metamask/swappable-obj-proxy": true,
"@metamask/network-controller>reselect": true,
"@metamask/rpc-errors": true,
"@metamask/utils": true,
"eslint>fast-deep-equal": true,
"reselect": true,
"uri-js": true,
"uuid": true
}
Expand Down Expand Up @@ -1756,13 +1756,6 @@
"semver": true
}
},
"@metamask/network-controller>reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"@metamask/notification-controller>nanoid": {
"globals": {
"crypto.getRandomValues": true
Expand Down Expand Up @@ -5138,6 +5131,13 @@
"@babel/runtime": true
}
},
"reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"semver": {
"globals": {
"console.error": true
Expand Down
16 changes: 8 additions & 8 deletions lavamoat/browserify/main/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -1694,10 +1694,10 @@
"@metamask/network-controller>@metamask/eth-block-tracker": true,
"@metamask/network-controller>@metamask/eth-json-rpc-infura": true,
"@metamask/network-controller>@metamask/swappable-obj-proxy": true,
"@metamask/network-controller>reselect": true,
"@metamask/rpc-errors": true,
"@metamask/utils": true,
"eslint>fast-deep-equal": true,
"reselect": true,
"uri-js": true,
"uuid": true
}
Expand Down Expand Up @@ -1756,13 +1756,6 @@
"semver": true
}
},
"@metamask/network-controller>reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"@metamask/notification-controller>nanoid": {
"globals": {
"crypto.getRandomValues": true
Expand Down Expand Up @@ -5138,6 +5131,13 @@
"@babel/runtime": true
}
},
"reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"semver": {
"globals": {
"console.error": true
Expand Down
16 changes: 8 additions & 8 deletions lavamoat/browserify/mmi/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -1786,10 +1786,10 @@
"@metamask/network-controller>@metamask/eth-block-tracker": true,
"@metamask/network-controller>@metamask/eth-json-rpc-infura": true,
"@metamask/network-controller>@metamask/swappable-obj-proxy": true,
"@metamask/network-controller>reselect": true,
"@metamask/rpc-errors": true,
"@metamask/utils": true,
"eslint>fast-deep-equal": true,
"reselect": true,
"uri-js": true,
"uuid": true
}
Expand Down Expand Up @@ -1848,13 +1848,6 @@
"semver": true
}
},
"@metamask/network-controller>reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"@metamask/notification-controller>nanoid": {
"globals": {
"crypto.getRandomValues": true
Expand Down Expand Up @@ -5230,6 +5223,13 @@
"@babel/runtime": true
}
},
"reselect": {
"globals": {
"WeakRef": true,
"console.warn": true,
"unstable_autotrackMemoize": true
}
},
"semver": {
"globals": {
"console.error": true
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@
"redux": "^4.0.5",
"redux-thunk": "^2.3.0",
"remove-trailing-slash": "^0.1.1",
"reselect": "^3.0.1",
"reselect": "^5.1.1",
"ses": "^1.1.0",
"simple-git": "^3.20.0",
"single-call-balance-checker-abi": "^1.0.0",
Expand Down
2 changes: 2 additions & 0 deletions shared/modules/selectors/smart-transactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export const getSmartTransactionsOptInStatusInternal = createSelector(
* @returns true if the user has explicitly opted in, false if they have opted out,
* or null if they have not explicitly opted in or out.
*/
// @ts-expect-error TODO: Fix types for `getSmartTransactionsOptInStatusInternal` once `getPreferences is converted to TypeScript
export const getSmartTransactionsOptInStatusForMetrics = createSelector(
getSmartTransactionsOptInStatusInternal,
(optInStatus: boolean): boolean => optInStatus,
Expand All @@ -96,6 +97,7 @@ export const getSmartTransactionsOptInStatusForMetrics = createSelector(
* @param state
* @returns
*/
// @ts-expect-error TODO: Fix types for `getSmartTransactionsOptInStatusInternal` once `getPreferences is converted to TypeScript
export const getSmartTransactionsPreferenceEnabled = createSelector(
getSmartTransactionsOptInStatusInternal,
(optInStatus: boolean): boolean => {
Expand Down
4 changes: 2 additions & 2 deletions shared/modules/selectors/util.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { TransactionStatus } from '@metamask/transaction-controller';
import { isEqual } from 'lodash';
import { createSelectorCreator, defaultMemoize } from 'reselect';
import { createSelectorCreator, lruMemoize } from 'reselect';

export const createDeepEqualSelector = createSelectorCreator(
defaultMemoize,
lruMemoize,
isEqual,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ const SnapAuthorshipPill: React.FC<SnapAuthorshipPillProps> = ({
onClick,
}) => {
const { name: snapName } = useSelector((state) =>
// @ts-expect-error ts is picking up the wrong type for the selector
getSnapMetadata(state, snapId),
);

Expand Down
1 change: 0 additions & 1 deletion ui/components/app/snaps/snap-icon/snap-icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export const SnapIcon: FunctionComponent<SnapIconProps> = ({
);

const { name: snapName } = useSelector((state) =>
/* @ts-expect-error wrong type on selector. */
getSnapMetadata(state, snapId),
);

Expand Down
42 changes: 27 additions & 15 deletions ui/ducks/bridge/selectors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ import {
import { mockNetworkState } from '../../../test/stub/networks';
import mockErc20Erc20Quotes from '../../../test/data/bridge/mock-quotes-erc20-erc20.json';
import mockBridgeQuotesNativeErc20 from '../../../test/data/bridge/mock-quotes-native-erc20.json';
import { SortOrder } from '../../pages/bridge/types';
import {
QuoteMetadata,
QuoteResponse,
SortOrder,
} from '../../pages/bridge/types';
import {
getAllBridgeableNetworks,
getBridgeQuotes,
Expand Down Expand Up @@ -722,9 +726,11 @@ describe('Bridge selectors', () => {
{ valueInCurrency: new BigNumber('0.156562871410260918428') },
{ valueInCurrency: new BigNumber('0.33900008283534602') },
];
result.sortedQuotes.forEach((quote, idx) => {
expect(quote.cost).toStrictEqual(EXPECTED_SORTED_COSTS[idx]);
});
result.sortedQuotes.forEach(
(quote: QuoteMetadata & QuoteResponse, idx: number) => {
expect(quote.cost).toStrictEqual(EXPECTED_SORTED_COSTS[idx]);
},
);
expect(result).toStrictEqual({
sortedQuotes: expect.any(Array),
recommendedQuote: {
Expand Down Expand Up @@ -827,9 +833,11 @@ describe('Bridge selectors', () => {
{ valueInCurrency: new BigNumber('0.15656287141025952') },
{ valueInCurrency: new BigNumber('0.33900008283534464') },
];
result.sortedQuotes.forEach((quote, idx) => {
expect(quote.cost).toStrictEqual(EXPECTED_SORTED_COSTS[idx]);
});
result.sortedQuotes.forEach(
(quote: QuoteMetadata & QuoteResponse, idx: number) => {
expect(quote.cost).toStrictEqual(EXPECTED_SORTED_COSTS[idx]);
},
);

expect(result).toStrictEqual({
sortedQuotes: expect.any(Array),
Expand Down Expand Up @@ -898,14 +906,18 @@ describe('Bridge selectors', () => {
'381c23bc-e3e4-48fe-bc53-257471e388ad',
);
expect(sortedQuotes).toHaveLength(2);
sortedQuotes.forEach((quote, idx) => {
expect(
quoteMetadataKeys.every((k) => Object.keys(quote ?? {}).includes(k)),
).toBe(true);
expect(quote?.quote.requestId).toStrictEqual(
mockBridgeQuotesNativeErc20[idx]?.quote.requestId,
);
});
sortedQuotes.forEach(
(quote: QuoteMetadata & QuoteResponse, idx: number) => {
expect(
quoteMetadataKeys.every((k) =>
Object.keys(quote ?? {}).includes(k),
),
).toBe(true);
expect(quote?.quote.requestId).toStrictEqual(
mockBridgeQuotesNativeErc20[idx]?.quote.requestId,
);
},
);
});

it('should sort quotes by ETA', () => {
Expand Down
Loading

0 comments on commit 6cc3672

Please sign in to comment.