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

Ensure that state changes are applied immediately on mount #256

Merged
merged 7 commits into from Jun 5, 2019

Conversation

kitten
Copy link
Member

@kitten kitten commented Jun 4, 2019

Fix #245
Fix #209 (See 17feeca)

It seems that in the current version React does
trigger two renders in our case.

We call executeQuery immediately which has a couple
of setStates. These are batched, it seems, but
do trigger a second update of the underlying
component, which is a shame.

Instead, we know that the initial executeQuery can
just safely merge its results into the initial
state, if it's during the initial mount.

This is why useImmediateState is added, which does
just that. Not the nicest way to solve this, but
the safest until React ships batched updates by
default.

This is basically a solution that will continue to
ensure that we won't have to add any state-related
stuff outside of executeQuery

@kitten
Copy link
Member Author

kitten commented Jun 4, 2019

cc @JoviDeCroock fyi: We still get a double render on mount since it seems that React will schedule a batched update on setState during initial mount instead of aborting to render that component, so this makes set State mutable during initial mount

Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Nice work, this really makes me wonder how you discover all these things :o lots to learn on my part it seems

src/hooks/useImmediateState.ts Outdated Show resolved Hide resolved
src/hooks/useImmediateState.ts Show resolved Hide resolved
@kitten
Copy link
Member Author

kitten commented Jun 4, 2019

@andyrichardson Added some tests for useImmediateState and an additional one for useImmediateEffect. They should both ensure that these hooks behave as we expect, so like a normal useState and useEffect respectively apart from the special behaviour on initial mount.

It seems that in the current version React does
trigger *two* renders in our case.

We call executeQuery immediately which has a couple
of setStates. These are batched, it seems, but
do trigger a second update of the underlying
component, which is a shame.

Instead, we know that the initial executeQuery can
just safely merge its results into the initial
state, if it's during the initial mount.

This is why useImmediateState is added, which does
just that. Not the nicest way to solve this, but
the safest until React ships batched updates by
default.
@kitten kitten changed the title Ensure that setState are immediate on mount Ensure that state changes in useQuery are applied immediately on mount Jun 4, 2019
This adds the additionalHooks option to eslint-plugin-react-hooks
that will run the exhaustive-deps check on useImmediateEffect as
if it was just the regular useEffect hook.
@kitten kitten changed the title Ensure that state changes in useQuery are applied immediately on mount Ensure that state changes are applied immediately on mount Jun 4, 2019
@andyrichardson andyrichardson merged commit 950ef7d into master Jun 5, 2019
@andyrichardson andyrichardson deleted the feat/sync-initial-state branch June 5, 2019 10:30
"react-hooks/exhaustive-deps": [
"warn",
{
"additionalHooks": "useImmediateEffect"
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️this really is such a nice surprise!

setState(action);
} else if (typeof action === 'function') {
const update = (action as any)(initialState.current);
Object.assign(initialState.current, update);
Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, totally could be missing something, but don't we need to do the assignment to initialState.current here to ensure the mutable ref gets updated, i.e.

initialState.current = Object.assign(initialState.current, update);

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, the first argument is being assigned to mutably 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, I forgot that Object.assign mutates the original object 👍

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.

fetching initially false? useMutation value should be constant
4 participants