-
Notifications
You must be signed in to change notification settings - Fork 670
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
Update Docs for v5 changes. #636
Conversation
…on signature is unsafe.
- Also add some of the memoization related types to exports.
- Add documentation regarding new features. - Redo `CodeSandbox` examples to align better with documentation. Solves reduxjs#598. - Fix GitHub workflow badge URL reduxjs#628. - Add instructions for `bun` and `pnpm`. Solves reduxjs#621. - Fix minor type issues regarding `createStructuredSelector`. Solves reduxjs#499. - `argsMemoize` and `argsMemoizeOptions` solve reduxjs#359. - Remove `Redux` legacy patterns including `connect` and `switch` statements from docs. Solves reduxjs#515. - Replace legacy code patterns with modern syntax like `useSelector` and functional components. - Implementation of `argsMemoize` solves reduxjs#376. - Document order of execution in `Reselect`. Solves reduxjs#455. - Add benchmarks to confirm the info inside the documentation. - Add more type tests with `vitest`. - Fix more hover preview issues with types.
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 c536318:
|
- Add `@typescript/analyze-trace` to measure TS performance bottlenecks.
- Add `IfUnknown` utility type. - Add `FallbackIfUnknown` utility type. - Fix TS issue related to `OutputSelector` which was stemming from `ExtractReturnType`.
- Add JSDocs for: * `createCurriedSelector` * `createCurriedSelectorCreator` * `CreateCurriedSelector` * `Curried` utility type. * `CurriedOutputSelector`
- Add more examples to docs. - Fix badges in docs. - Fix Table of Contents. - Fix external references. - Fix internal links. - Make Terminology section toggled. - Remove `skortchmark9` chrome extension reference since it is no longer available.
- Add `examples.test.ts` file to test examples from docs. - Fix remaining benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only got through the first few sections so far - I'll come back and review the rest later. Wanted to pass on the initial feedback. Thanks for working on this!
README.md
Outdated
- Selectors can compute derived data, allowing Redux to store the minimal possible state. | ||
- Selectors are efficient. A selector is not recomputed unless one of its arguments changes. | ||
- Selectors are composable. They can be used as input to other selectors. | ||
- Selectors can compute derived data, allowing `Redux` to store the minimal possible state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ style note: we typically leave the name of the library as a proper capitalized name ("Reselect", "Redux", "React-Redux" "Redux Toolkit"), and specifically use backticks for the lower-case actual package name (reselect
, redux
, react-redux
, @reduxjs/toolkit
). Could you fix the names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sir, sorry about that. I will fix that right away!
README.md
Outdated
|
||
## Basic Usage | ||
|
||
`Reselect` exports a [`createSelector`] API, which generates memoized selector functions. [`createSelector`] accepts one or more [`input selectors`], which extract values from arguments, and a [`combiner`] function that receives the extracted values and should return a derived value. If the generated [`output selector`] is called multiple times, the output will only be recalculated when the extracted values have changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋
- There's a bunch of inline code names that also have brackets. Are these supposed to be links?
- Typically inline code formatting is used for actual variable / function names. "combiner" and "input selector" are descriptive terms and shouldn't be formatted as code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes the brackets are links.
- I will fix the inline formatting.
Thanks for pointing these out!
README.md
Outdated
|
||
As you can see from the example above, `memoizedSelectCompletedTodos` does not run the second or third time, but we still get the same return value as last time. | ||
|
||
Another difference is that with `memoizedSelectCompletedTodos` the referential integrity of the return value is also maintained through multiple calls of the selector, but the same cannot be said about the first example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ the phrasing of this sentence feels a bit awkward. How about:
Another difference is that with `memoizedSelectCompletedTodos` the referential integrity of the return value is also maintained through multiple calls of the selector, but the same cannot be said about the first example. | |
In addition to skipping unnecessary recalculations, `memoizedSelectCompletedTodos` returns the existing result reference if there is no recalculation. This is important for libraries like React-Redux or React that often rely on reference equality checks to optimize UI updates. |
README.md
Outdated
|
||
<details><summary><b>Click to expand</b></summary> | ||
|
||
- <a name="selector-function"></a>[**`Selector Function`**](#selector-function): A function that takes the `Redux` store state (or a part of it) as an argument and returns data derived from that state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ I think it's worth keeping the "not actually required to use Redux" aspect here. How about:
- <a name="selector-function"></a>[**`Selector Function`**](#selector-function): A function that takes the `Redux` store state (or a part of it) as an argument and returns data derived from that state. | |
- <a name="selector-function"></a>[**`Selector Function`**](#selector-function): A function that accepts one or more JS values as input arguments, and derives a result. When used with Redux, the first argument is typically the entire Redux store state. |
README.md
Outdated
<details><summary><b>Click to expand</b></summary> | ||
|
||
- <a name="selector-function"></a>[**`Selector Function`**](#selector-function): A function that takes the `Redux` store state (or a part of it) as an argument and returns data derived from that state. | ||
- <a name="input-selectors"></a>[**`input selectors`**](#input-selectors): Basic selector functions used as building blocks for creating a memoized selector. They are passed as the first argument(s) to [`createSelector`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- <a name="input-selectors"></a>[**`input selectors`**](#input-selectors): Basic selector functions used as building blocks for creating a memoized selector. They are passed as the first argument(s) to [`createSelector`]. | |
- <a name="input-selectors"></a>[**`input selectors`**](#input-selectors): Basic selector functions used as building blocks for creating a memoized selector. They are passed as the first argument(s) to [`createSelector`], are called with all of the selector arguments, and extract values for use in the result function. |
README.md
Outdated
- <a name="input-selectors"></a>[**`input selectors`**](#input-selectors): Basic selector functions used as building blocks for creating a memoized selector. They are passed as the first argument(s) to [`createSelector`]. | ||
- <a name="output-selector"></a>[**`Output Selector`**](#output-selector): The actual memoized selectors created by [`createSelector`]. | ||
- <a name="result-function"></a>[**`Result Function (Combiner)`**](#result-function): The function that comes after the [`input selectors`]. It takes the [`input selectors`]' return values as arguments and returns a result. Otherwise known as the `combiner`. | ||
- <a name="combiner"></a>[**`Combiner`**](#combiner): A function that takes [`input selectors`]' return values as arguments and returns a result (This function comes after the [`input selectors`] inside [`createSelector`]). This term is somewhat interchangeably used with `Result Function` or `resultFunc`. But `combiner` is more of a general term and `resultFunc` is more specific to `Reselect`. So the `resultFunc` is a `combiner` but a `combiner` is not necessarily the same as `resultFunc`. For the sake of simplicity, they are used synonymously throughout this documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ if we're going to consolidate terms, I'd like to drop "combiner" entirely and just stick with "result function" (or possibly "output function").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, the only reason I included "combiner" is because of how it was referened internally. But you're right we can drop that term in the docs to avoid further confusion. I think we can just stick to "result function". "output function" can be confused with "output selector". What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I missed the use of "output selector" here. I don't think I like that usage tbh :) I personally have used that phrase to mean what we're now calling the "result function". If we have to distinguish things, I'd say "generated selector", maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I think "output selector" makes sense for the generated selector. It would also be consistent with how the types were named initially. The result function is not really a "selector" if you think about it. The way it is now we have "input selectors", then "result function" does the calculation, and we get an "output selector". What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's kind of like:
input -> calculation -> output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, I'm also not sold on the type names either, even though they've been there since early on :) "Parametric selector" has always confused me
Tbh, me neither, but I was just trying to stay consistent with the type names. And you're right about the "parametric selector", I'm not really a fan of it either. But they were there since the beginning and I thought that's how you would want me to name things in the docs as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to stick with either "generated selector" or "memoized selector" as the term for the function returned by
createSelector
.
"memoized selector" makes sense. However, since we have "input selectors" and if we have an "input" it makes sense that we would have an "output" as well. So I still think "output selector" makes the most sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we have 2 types that I would like to remove entirely if that's okay with you. ParametricSelector
and OutputParametricSelector
since Selector
and OutputSelector
support additional parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, go ahead and remove those. Breaking change, we'll have to document that, but might as well.
Per the "input/output" thing: I think I disagree because the "input selectors" are "selectors" (args in, derived value out even if it's usually just an extraction), and "inputs to createSelector
". The "result function" doesn't get any of the original args, so even though I've referred to it as an "output selector" in the past, that's really not a good usage and I shouldn't have called it that.
The actual generated selector isn't just about "output", though. It has all three pieces: the input arguments, the memoization, and calculating the final result. It's not just about "output". The overall signature is (...args) => derivedValue
, so it's a standard selector overall, it just happens to be a "memoized" selector that was "generated" by createSelector
rather than hand-written. So, I do think that "memoized" or "generated" are the best terms here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the "result function" is not an "output selector". I agree. That was one of the things that confused me when I first started working on the codebase. the "result function" was sometimes referred to as "the combiner" (which I think in terms of semantics makes the most sense). But it was occasionally referred to as the "output selector", but the genereated selector was also sometimes referred to as the "output selector". In the JSDocs I always called the final generated selector "a memoized output selector".
BTW I did a quick search in github on ParametricSelector
and OutputParametricSelector
seems like anybody was hardly using them so I think we'll be okay removing them, I will go ahead and document their removal.
I still think in terms of simplicity "output selector" makes sense. The "memoized" part is implied and kind of a given. Input -> result function -> output
sums up the flow of code as well. It's self explanatory in that Reselect goes through the "inputs" then "resultFunc" and gives an "output". It's linear and easier to digest no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, got some additional requests:
- We can limit terminology links to just the first time each term gets used, as well as maybe the first usage for each inside the API reference section
- Got some requests for reordering the API reference
- Several terms that are bolded or inline-code that don't need to be ("Since", "Example", etc)
- Can you split this into multiple PRs? this is too much to review at once :) and they're really at least 4 separate topics:
- Docs changes
- Types changes
- Benchmarks
createCurriedSelector
- If the results are the same, it returns the cached result without running the [`result function`]. | ||
- If the results differ, it runs the [`result function`]. | ||
|
||
This behavior is what we call **_Cascading Double-Layer Memoization_**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ which part are we describing as "cascading"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If argsMemoize
fails, Reselect "cascades" to memoize
. I will tweak the explanation to make it more clear.
README.md
Outdated
|
||
### Cascading Memoization | ||
|
||
<details><summary><b>Click to expand</b></summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ couple thoughts here:
I'd recently added this snippet under the "how do you pass in arguments?" section:
const finalSelector = (...args) => {
const extractedValues = inputFunctions.map(input => input(...args));
return output(...extractedValues);
}
I'd like to have that right under the ## How Does Reselect Work?
header, and one or two short paragraphs of explanation.
Also, I'd like to avoid having headers with no content at all. Let's have at least a sentence or two under each header with a basic explanation, then use the <details>
to let them dig in further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recently added this snippet under the "how do you pass in arguments?" section:
Honestly the snippet confused me, but I think I know what you're trying to convey. And I already had an explanation of what you're trying to get across with the snippet, this is what it looks like:
const selectAvailableItems = createSelector(
[
// First input selector extracts items from the state
(state: RootState) => state.items,
// Second input selector forwards the category argument
(state: RootState, category: string) => category,
// Third input selector forwards the ID argument
(state: RootState, category: string, id: number) => id
],
// Output selector uses the extracted items, category, and ID
(items, category, id) =>
items.filter(item => item.category === category && item.id !== id)
)
Internally Reselect is doing this:
// Input selector #1
const items = (state: RootState, category: string, id: number) => state.items
// Input selector #2
const category = (state: RootState, category: string, id: number) => category
// Input selector #3
const id = (state: RootState, category: string, id: number) => id
// result of `output selector`
const finalResult =
// The `Result Function`
items.filter(item => item.category === category && item.id !== id)
Also, I'd like to avoid having headers with no content at all. Let's have at least a sentence or two under each header with a basic explanation, then use the
to let them dig in further.
That makes a lot of sense, I will fix it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my example is meant to show that "all arguments get passed to all input selectors", specifically (which is why they all have to have compatible signatures).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I explained it:
-
Consistency in Arguments: Ensure that all positional arguments across [input selectors] are of the same type for consistency.
-
Selective Argument Usage: Design each selector to use only its relevant argument(s) and ignore the rest. This is crucial because all [input selectors] receive the same arguments that are passed to the [output selector].
Do you still want me to include your snippet as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've used variations of that snippet a few times and definitely want it in here.
README.md
Outdated
|
||
| Name | Description | | ||
| :----------------------- | :----------------------------------------------------------------------------------------------- | | ||
| `inputSelectors` | An array of [`input selectors`], can also be passed as separate arguments. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ We'll to update the terms consistently, but especially here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What "term" are we talking about here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, what was I thinking? :)
It was one of two things:
- We can skip wrapping every single use of one of the "Terminology" terms in links. Per one of my other comments, we only need to highlight those in the first usage (and we only want to use backticks for actual variable/function names)
- Making sure that whatever final terminology terms we settle on get used consistently, particularly in the API reference section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sir, I will fix the backticks and formatting issues.
README.md
Outdated
|
||
### createSelector(...inputSelectors | [inputSelectors], resultFunc, createSelectorOptions?) | ||
|
||
<details><summary><b>Description</b></summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ "Description", "Parameters", and "Returns" shouldn't be hidden inside of <details>
- we want readers to be able to see those directly as they scroll down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yessirrr. I will adjust that.
README.md
Outdated
| ------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| `resultFunc` | The final function passed to [`createSelector`]. Otherwise known as the `combiner`. | | ||
| `memoizedResultFunc` | The memoized version of `resultFunc`. | | ||
| `lastResult` | Returns The last result calculated by `memoizedResultFunc`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `lastResult` | Returns The last result calculated by `memoizedResultFunc`. | | |
| `lastResult` | Returns the last result calculated by `memoizedResultFunc`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the eyes of an eagle!
- **Customization Enhancements**: | ||
|
||
- Added the ability to pass an options object to [`createSelectorCreator`], allowing for customized `memoize` and `argsMemoize` functions, alongside their respective options (`memoizeOptions` and `argsMemoizeOptions`). | ||
- The [`createSelector`] function now supports direct customization of `memoize` and `argsMemoize` within its options object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ I'd put this one above the createSelectorCreator
customization, really :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your wish is my command :)
src/createCurriedSelectorCreator.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ I'd like to drop createCurriedSelector
from this PR entirely, for a few reasons:
- This PR is huge already, and I'd like it to focus on just the docs changes
- Not yet sure it's something we actually want to ship
- It's something we can discuss after 5.0 is done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think it will be helpful, it's kind of overdue actually. Would I be able give you some more confidence and perhaps change your mind by writing an insane amount of tests? It is basically a variation of createSelector
that serves as a utility function to make our lives easier. It doesn't produce a breaking change, and I labled it as "experimental". It just seems like an appropriate API to include with a major version bump no? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whether it's helpful or not, it doesn't make sense to be included in a PR named "update docs for v5 changes"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EskiMojo14 Yessirrr you are correct, I will split them into multiple PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can discuss it after RTK 2.0 / Reselect 5.0 is out. It's a new API, so it's something we can always release in a minor if we do decide to add it. Right now I'm trying to stop adding or changing anything that's not strictly necessary to get RTK 2.0 out the door :) (already punted on createSlice.selectorFactories
, etc, so that's not holding things up.)
src/types.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Can you also move all the types changes to a separate PR as well? That would make this easier to review and keep track of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really "change" much of the types. I did added some more type expansions for hover-previews, and fixed a small inference issue with OutputSelector
that was coming from ExtractReturnType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, can you still move those to a separate PR just so they're easier to read through?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Heh, yep, also please move new benchmarks to a separate PR as well :)
Can probably split this into at least 4 PRs:
- README docs changes
- Types changes
- Benchmarks
createCurriedSelector
…StructuredSelector`.
README.md
Outdated
return currentVal === previousVal | ||
<details><summary><b>Top Level Selectors Pattern</b></summary> | ||
|
||
This pattern simplifies the creation of selectors in your application. Start by defining a generic type for your top-level selectors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ I think this should actually be mostly unnecessary , since createSlice
in RTK 2.0 now adds a mySlice.selectSlice
function that assumes the slice is mounted in the default name spot. In other words:
const todosSlice = createSlice({
name: "todos",
initialState,
reducer
})
// later
// this returns state.todos, since that was the `slice.name` field
const todosState = todosSlice.selectSlice(store.getState())
README.md
Outdated
|
||
The `...memoizeOptions` rest parameters are zero or more configuration options to be passed to `memoizeFunc`. The selectors `resultFunc` is passed as the first argument to `memoize` and the `memoizeOptions` are passed as the second argument onwards: | ||
<details><summary><b>Answer</b></summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ Actually going to remove most of these <details>
sections from the FAQs, except for maybe a couple longer answers
Awright, let's get this in! |
This makes me so happy |
This PR mostly focuses on updating the documentation for v5.
This includes removing any examples that use legacy syntax such as
connect
orswitch
statements.Currently there are some confusing jargon related to Reselect, things like "output selector" and "result function" are sometimes used interchangeably which can cause more confusion down the line. To mitigate that problem I also added a "Terminology" section. There were a lot of changes made to the documentation so far but there will be some more coming. I will probably make some more examples with CodeSandbox to demonstrate how the new features work.
bun
andpnpm
. Solves docs: add instructions for bun and pnpm #621.argsMemoize
andargsMemoizeOptions
solve Use provided memoize options in outer memoize call #359.connect
andswitch
statements from docs. Solves Rewrite the readme to show "Modern Redux" patterns and practices #515.useSelector
and functional components.argsMemoize
solves Regression when using lodash for an unbounded cache and reselect 4.0 #376.