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

Reduce identityFunctionCheck false positives #660

Merged

Conversation

Methuselah96
Copy link
Member

@Methuselah96 Methuselah96 commented Dec 10, 2023

This PR attempts to reduce the number of false positives identified by identityFunctionCheck by making two changes:

  1. Only produce a warning if the result function is only passed one argument.
  2. Compare the first argument passed to the result function with the value returned by the result function. There are some edge cases where this could prevent some false positives, although admittedly, they're likely rare (one hypothetical edge case is added as a test).

Copy link

netlify bot commented Dec 10, 2023

Deploy Preview for reselect-docs canceled.

Name Link
🔨 Latest commit 997534f
🔍 Latest deploy log https://app.netlify.com/sites/reselect-docs/deploys/657642a3ef7d4f0008ba56a5

@Methuselah96 Methuselah96 changed the title Reduce identityFunctionCheck false positives Reduce identityFunctionCheck false positives Dec 10, 2023
Copy link

codesandbox-ci bot commented Dec 10, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 997534f:

Sandbox Source
Vanilla Typescript Configuration

To get the performance tests to pass (?!)
@aryaemami59
Copy link
Contributor

This seems like a good idea, the only problem is now something like this won't generate a warning:

const badSelector = createSelector(
  [(state: RootState) => state.todos, (state: RootState) => state.alerts],
  (todos, alerts) => todos
)

I actually thought about doing exactly what you implemented when I was initially working on it, there is definitely a trade off, not sure which one is the way to go.

@Methuselah96
Copy link
Member Author

Methuselah96 commented Dec 17, 2023

Agreed, there's no way to make it perfect, but I would definitely lean towards this being a better tradeoff.

  1. This still correctly warns for all the practical examples given in Consider dev mode checks for x => x result functions #642.
  2. This check has so far produced at least 8 false positives and 0 true positives in my work codebase. Each warning needs to be investigated and takes time to either individually silence, or rework (for each person who runs across them).
  3. The cost of false negatives are fairly low. If the check misses more rare forms of the identity function, like the example you provided, it's not that big a deal. We don't have practical examples of people writing identity functions like that in the wild, whereas we do have clear practical examples for a simple identity functions with one arg, and for conditional selectors with multiple args, both of which seem much more common.

All-in-all, it seems clear to me that this provides a better balance for catching true issues, while avoiding false positives.

@aryaemami59
Copy link
Contributor

Yeah, currently I'm working on eslint-plugin for Reselect and RTK, and I think we can go with the solution you provided, and then the other edge cases can be caught using static code analysis. This way we can give accurate warnings without being too strict about it.

@markerikson markerikson merged commit 0837509 into reduxjs:master Dec 17, 2023
19 checks passed
@Methuselah96 Methuselah96 deleted the improve-identity-function-check branch December 17, 2023 14:53
descope bot referenced this pull request in descope/descope-js Mar 19, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [reselect](https://togithub.com/reduxjs/reselect) | dependencies |
minor | [`5.0.1` ->
`5.1.0`](https://renovatebot.com/diffs/npm/reselect/5.0.1/5.1.0) |

---

### Release Notes

<details>
<summary>reduxjs/reselect (reselect)</summary>

###
[`v5.1.0`](https://togithub.com/reduxjs/reselect/releases/tag/v5.1.0)

[Compare
Source](https://togithub.com/reduxjs/reselect/compare/v5.0.1...v5.1.0)

This **minor release**:

- Adds a new `createSelector.withTypes<RootState>()` and
`createStructuredSelector.withTypes<RootState>()` API
- Deprecates the `TypedStructuredSelectorCreator` type introduced in 5.0
- Aims to reduce false positives in `identityFunctionCheck` by only
running if the output selector is passed one argument
- Fixes a bug with `weakMapMemoize`'s `resultEqualityCheck` when used
with a primitive result.

##### `withTypes`

Most commonly, selectors will accept the root state of a Redux store as
their first argument. `withTypes` allows you to specify what that first
argument will be ahead of creating the selector, meaning it doesn't have
to be specified.

```ts
// previously
export const selectPostById = createSelector(
  [
    (state: RootState) => state.posts.entities,
    (state: RootState, id: number) => id,
  ],
  (entities, id) => entities[id],
);
// now
export const createAppSelector = createSelector.withTypes<RootState>();

export const selectPostById = createAppSelector(
  [(state) => state.posts.entities, (state, id: number) => id],
  (entities, id) => entities[id],
);
```

##### Known limitations

Due to a Typescript issue, inference of the output selector's parameters
only works with `withTypes` when using an array of input selectors.

If using the variadic version, you can either wrap your input selectors
in an array instance (as above), or annotate the parameters manually.

```ts
export const createAppSelector = createSelector.withTypes<RootState>();

export const selectPostById = createAppSelector(
  (state) => state.posts.entities, 
  (state, id: number) => id,
  // parameters cannot be inferred, so need annotating
  (entities: Record<number, Post>, id: number) => entities[id],
);
```

##### What's Changed

- Reduce `identityFunctionCheck` false positives by
[@&#8203;Methuselah96](https://togithub.com/Methuselah96) in
[https://github.com/reduxjs/reselect/pull/660](https://togithub.com/reduxjs/reselect/pull/660)
- Fix cut content inside TOC of docs by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/664](https://togithub.com/reduxjs/reselect/pull/664)
- Remove redundant Svg requires from components in docs by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/665](https://togithub.com/reduxjs/reselect/pull/665)
- Fix `_lastResult.deref` is not a function (it is undefined) in React
Native and Expo applications by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/671](https://togithub.com/reduxjs/reselect/pull/671)
- Update getting-started.mdx by
[@&#8203;cchaonie](https://togithub.com/cchaonie) in
[https://github.com/reduxjs/reselect/pull/672](https://togithub.com/reduxjs/reselect/pull/672)
- Update createSelectorCreator.mdx with correct defaults by
[@&#8203;lukeapage](https://togithub.com/lukeapage) in
[https://github.com/reduxjs/reselect/pull/674](https://togithub.com/reduxjs/reselect/pull/674)
- Introduce pre-typed `createSelector` via
`createSelector.withTypes<RootState>()` method by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/673](https://togithub.com/reduxjs/reselect/pull/673)
- Bump RTK and React-Redux to latest versions by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/676](https://togithub.com/reduxjs/reselect/pull/676)
- add publish job by
[@&#8203;EskiMojo14](https://togithub.com/EskiMojo14) in
[https://github.com/reduxjs/reselect/pull/677](https://togithub.com/reduxjs/reselect/pull/677)
- Wrap up implementation of `TypedStructuredSelectorCreator` by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/667](https://togithub.com/reduxjs/reselect/pull/667)
- Introduce pre-typed `createStructuredSelector` via
`createStructuredSelector.ts.withTypes<RootState>()` method by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/678](https://togithub.com/reduxjs/reselect/pull/678)
- Bump `vitest` to v1 by
[@&#8203;aryaemami59](https://togithub.com/aryaemami59) in
[https://github.com/reduxjs/reselect/pull/668](https://togithub.com/reduxjs/reselect/pull/668)

##### New Contributors

- [@&#8203;Methuselah96](https://togithub.com/Methuselah96) made their
first contribution in
[https://github.com/reduxjs/reselect/pull/660](https://togithub.com/reduxjs/reselect/pull/660)
- [@&#8203;cchaonie](https://togithub.com/cchaonie) made their first
contribution in
[https://github.com/reduxjs/reselect/pull/672](https://togithub.com/reduxjs/reselect/pull/672)

**Full Changelog**:
reduxjs/reselect@v5.0.1...v5.1.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3MC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants