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

perf: replace useState with useReducer #5

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

leonardodino
Copy link
Contributor

@leonardodino leonardodino commented Feb 27, 2019

follow up to #3 (perf optimization)

also: bundle size will be even smaller (not that is a concern 🤣)

this avoids creating a new function on each render,
the identity value for the dispatch function stays stable.

passing dispatcher down is encouraged because of "ref stability".

added some tests to guarantee void returns from function,
as I had to do some coercion. (dispatcher function is too specific)

this is not a breaking change, and compatible with both alpha and stable react.
(the alpha lacks the third argument to useReducer, this doesn't use it.)


update: regarding the other, even shorter, proposal:
the reducer proposed here seems more "reliable" as the setState will always return undefined.

React may bail out in the future, if it doesn't already.

update 2: yeps, react broke the () => useState()[1]:

should update a component:

AssertionError: expected 1 to equal 2

-1
+2

follow up to quisido#3 (perf optimization)
this avoids creating a new function on each render,
the identity value for the dispatch function stays stable.

[passing dispatcher down is encouraged](https://reactjs.org/docs/hooks-faq.html#how-to-avoid-passing-callbacks-down)
because of "ref stability".

added some tests to guarantee void returns from function,
as I had to do some coercion because dispatcher function is specific.
package.json Outdated Show resolved Hide resolved
return forceUpdate;
};
const useForceUpdate: VoidFunctionCreator = () => (
useReducer(reducer, true)[1] as VoidFunction
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not as short or memory efficient, but I think destructuring first to [ , dispatch ] will make the repo more readable and inviting to contributors. f(x, y)[z] looks like a lot of magic that may not be intuitive to learners.

Why is dispatch being overridden as a VoidFunction? What is it's default return type?

Copy link
Contributor Author

@leonardodino leonardodino Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not as short or memory efficient, but I think destructuring first to [ , dispatch ] will make the repo more readable and inviting to contributors. f(x, y)[z] looks like a lot of magic that may not be intuitive to learners.

💯% agreed.

Why is dispatch being overridden as a VoidFunction? What is it's default return type?

yeah, I saw that the assignment was a bit of a hack.
I'm fairly new to TypeScript, and don't know how to solve that.
It doesn't compile without overriding it.

the type is React.Dispatch<void> (aka: (value: void) => void).

Type 'Dispatch<void>' is not assignable to type 'VoidFunction'.

I can make the hook return this type, but that, I think, is exposing implementation details, and it's incompatible with VoidFunction because it receives an argument.

Copy link
Owner

@quisido quisido Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: We do want this changed if it receives an argument. I'll keep looking into the definition. The fact that action is defined as void should be enough for it to realize there is no argument.

We can replace VoidFunction with Dispatch<void>.

I think

  • import { Dispatch, useReducer } from 'react';
  • delete VoidFunctionCreator
  • change useForceUpdate's definition to be (): Dispatch<void>

@quisido
Copy link
Owner

quisido commented Feb 27, 2019

this avoids creating a new function on each render,
the identity value for the dispatch function stays stable.

This is subtle. Took me a moment to see the change. I really like it.

update: regarding the other, even shorter, proposal:
update 2: yeps, react broke the () => useState()[1]:

As much as I like unnecessarily short code, this is precisely why I didn't implement it. React behavior in the future isn't predictable enough to say that calling setState will always trigger a re-render. I wasn't super happy about toggling back and forth between booleans, because I feared a race condition for a shallow comparison, where it toggled twice before a re-render/animation frame, and React decides not to re-render because the state is now the same again.

That isn't the case for how it's treated right now, but I didn't want to have to refactor for future releases. Oh well. People didn't like the integer thing. C'est la vie.

@leonardodino
Copy link
Contributor Author

it was news to me too.

I came about it, when I was writing a useEffect that dispatched conditionally something.
I didn't knew if I should add dispatch to the list of "dependencies".

turn's out it's stable (I quick tested with a ref), then looked at the eslint-plugin,
it's a special case there. (:

It's very subtle indeed. 🤓

return forceUpdate;
const useForceUpdate: VoidFunctionCreator = () => {
const [, dispatch] = useReducer(reducer, true);
return dispatch as VoidFunction;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will wait on feedback regarding the as VoidFunction, maybe there's a way around it.

no hurries to merge, it's not even a direct dependency of mine :P
I stumbled upon it while reading your use-react-router.

@quisido quisido self-assigned this Feb 27, 2019
@quisido quisido added the enhancement New feature or request label Feb 27, 2019
@quisido quisido merged commit a94c6f9 into quisido:master Feb 28, 2019
@quisido
Copy link
Owner

quisido commented Feb 28, 2019

Thanks for the PR. After merging, I did a little tampering to see what I could find. I had to use useMemo to get rid of that mandatory parameter. TypeScript's as could get rid of the IntelliSense and transpilation errors, but it wouldn't pass unit tests on creating a function with no arguments. useMemo does require a function be created every render, which is a little overhead, but it doesn't change dispatch's functional reference each render, which will allow it to be passed as a prop without changing that prop's reference each render; and I think that was the primary benefit of the PR.

I really appreciate the extra eyes on the project. If you have any other ideas or concerns, feel free to share them. Thanks again!

@quisido quisido added this to the 1.0.3 milestone Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants