Skip to content
This repository has been archived by the owner on Sep 18, 2020. It is now read-only.

Always change language where necessary if path changes #3

Merged
merged 2 commits into from
Dec 13, 2019

Conversation

deanc
Copy link
Contributor

@deanc deanc commented Dec 12, 2019

A fix for this issue:
#1


useEffect(() => {
i18n.changeLanguage(locale)
}, [location, i18n, locale])
Copy link
Owner

Choose a reason for hiding this comment

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

What's the idea in mind of adding i18n in the dependencies list for the side effect?

Copy link
Contributor Author

@deanc deanc Dec 13, 2019

Choose a reason for hiding this comment

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

Ah, the latest builds of gatsby use eslint which gives this warning:

warn ESLintError:
/Users/dc/Projects/finbb-frontpage/src/layouts/index.js
  16:6  warning  React Hook useEffect has missing dependencies: 'i18n' and 'locale'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

After a bit of research the recommendation is that you should include all dependencies that are inside the useEffect hook inside that second array, as they can change from the outside too, which in theory should change how they are used inside the hook.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright, thanks, that's cool :)
I personally don't like putting whole objects in a dep list, but if the linter say so :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but think of it this way. It's used inside the hook, so it is a dependency.

@kalinchernev kalinchernev merged commit 51ec4b2 into kalinchernev:master Dec 13, 2019
@kalinchernev
Copy link
Owner

kalinchernev commented Dec 13, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants