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

React has detected a change in the order of Hooks called by EmotionCssPropInternal #2423

Closed
tills13 opened this issue Jul 8, 2021 · 6 comments · Fixed by #2424
Closed

React has detected a change in the order of Hooks called by EmotionCssPropInternal #2423

tills13 opened this issue Jul 8, 2021 · 6 comments · Fixed by #2424

Comments

@tills13
Copy link
Contributor

tills13 commented Jul 8, 2021

Current behavior:

When using a conditional css which consumes theme, the following React error is produced when the condition changes from false to true:

React has detected a change in the order of Hooks called by EmotionCssPropInternal

To reproduce:

https://codesandbox.io/s/emotion-issue-template-forked-ettgx?file=/src/index.js

In particular, the line

<Title css={toggle && toggledCss}>

when toggle changes from false to true, the error above is produced. It only happens the first time it's toggled, I assume this is because the css or resulting wrapped component is cached somewhere? -- not sure and not sure if relevant. I did some digging in Emotion and found the offending line to be this one. This line also hinted at a potential solution:

<Title css={[toggle && toggledCss]}>

Now... I'm not really sure which styles of css usage are supported. The documentation is lacking in this regard and much of our usage is inferred from TypeScript types. So really, my question is: if this is invalid, how should I write something like this? A less contrived example might be conditionally applying hover, active, etc states depending on whether or not a button is disabled.

Expected behavior:

If this style is in fact supported, no React warning should be produced.

Environment information:

  • react version: 16.8.6+
  • @emotion/react version: 11.0+
@Andarist
Copy link
Member

Andarist commented Jul 8, 2021

Well, your code looks reasonable and valid. We are trying to minimize how many components "subscribe" to the theme context in the implementation and this has backfired here. I wonder - maybe this is a premature optimization anyway? @mitchellhamilton thoughts?

@emmatown
Copy link
Member

emmatown commented Jul 8, 2021

Yeah, unconditionally using the theme context sgtm

@tills13
Copy link
Contributor Author

tills13 commented Jul 8, 2021

I wouldn't mind putting a PR together for this unless either of you would like to handle it yourselves.

@emmatown
Copy link
Member

emmatown commented Jul 9, 2021

A PR would be great

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Jul 19, 2021

I just ran into this issue in our platform, except for me the same problem occurs regardless if I use an array and always on initial render, even if the condition does not change (I tested with a constant).

Quickly looking at the line that @tills13 found, it seems to me that this line breaks the first Rules of Hooks, namely that hooks should never be called conditionally.

Looking forward to get the PR merged, I will make the change as a patch and try it out myself as well.
Edit: Yep, seems like it did the trick, still curious if this was done merely as a premature optimization or if there is a big performance difference.

@Andarist
Copy link
Member

Sorry for the delay - I hope to merge the mentioned PR this week. Didn't have much time last week to get to this.

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

Successfully merging a pull request may close this issue.

4 participants