-
Notifications
You must be signed in to change notification settings - Fork 672
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
Reduce identityFunctionCheck
false positives
#660
Conversation
✅ Deploy Preview for reselect-docs canceled.
|
identityFunctionCheck
false positives
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:
|
To get the performance tests to pass (?!)
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. |
Agreed, there's no way to make it perfect, but I would definitely lean towards this being a better tradeoff.
All-in-all, it seems clear to me that this provides a better balance for catching true issues, while avoiding false positives. |
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. |
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 [@​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 [@​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 [@​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 [@​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 [@​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 [@​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 [@​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 [@​aryaemami59](https://togithub.com/aryaemami59) in [https://github.com/reduxjs/reselect/pull/676](https://togithub.com/reduxjs/reselect/pull/676) - add publish job by [@​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 [@​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 [@​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 [@​aryaemami59](https://togithub.com/aryaemami59) in [https://github.com/reduxjs/reselect/pull/668](https://togithub.com/reduxjs/reselect/pull/668) ##### New Contributors - [@​Methuselah96](https://togithub.com/Methuselah96) made their first contribution in [https://github.com/reduxjs/reselect/pull/660](https://togithub.com/reduxjs/reselect/pull/660) - [@​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>
This PR attempts to reduce the number of false positives identified by
identityFunctionCheck
by making two changes: