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

useSteps(): only update setStep when needed #774

Merged

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented May 9, 2019

@bpierre bpierre requested a review from sohkai May 9, 2019 12:14
@bpierre bpierre mentioned this pull request May 9, 2019
@@ -42,8 +45,6 @@ const LocalIdentities = ({
return <EmptyLocalIdentities onImport={onImport} />
}

const { identityEvents$ } = React.useContext(IdentityContext)
const { showLocalIdentityModal } = React.useContext(LocalIdentityModalContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to avoid making this change until after #768 was merged, so we don't have too many conflicts :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted :+1:

This reverts commit 68f0749.
},
[setDirection, _setStep, step]
[steps]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to include the reducer's updateStep because it'll never change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, setSomething from useState() and dispatch from useReducer() are both guaranteed to never update [1], and don’t need to be added to the dependencies list.

[1] Search for “guarantees” in these two sections:

https://reactjs.org/docs/hooks-reference.html#usestate
https://reactjs.org/docs/hooks-reference.html#usereducer

@bpierre bpierre changed the title Eslint add hook rules dependencies fix useSteps(): only update setStep when needed May 9, 2019
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Awesome, sounds like we've learned something new about hooks :).

Although it's still baffling why the little hacks I was doing didn't work specifically for a single case of setting the steps to 1 :(

@bpierre
Copy link
Contributor Author

bpierre commented May 9, 2019

Awesome, sounds like we've learned something new about hooks :).
Yes! 👍

Although it's still baffling why the little hacks I was doing didn't work specifically for a single case of setting the steps to 1 :(

Yes and I’m not a big fan of the solution as it is… but at least the complexity stays in the hook itself, which is nice. Maybe we should move every custom hook in its own file, so that reducers, initial states etc. declared outside of the hook itself wouldn’t be in the same module than other hooks.

@bpierre bpierre merged commit 3912081 into eslint-add-hook-rules May 9, 2019
@bpierre bpierre deleted the eslint-add-hook-rules--dependencies-fix branch May 9, 2019 13:05
@sohkai
Copy link
Contributor

sohkai commented May 9, 2019

Sounds reasonable to have a hooks/ folder to hold all the hooks

sohkai added a commit that referenced this pull request May 14, 2019
* Add eslint rules for react-hooks
* useSteps(): only update setStep when needed (#774)
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.

2 participants