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

Call all functions twice in StrictMode #18426

Closed
eps1lon opened this issue Mar 29, 2020 · 15 comments · Fixed by #18430
Closed

Call all functions twice in StrictMode #18426

eps1lon opened this issue Mar 29, 2020 · 15 comments · Fixed by #18430

Comments

@eps1lon
Copy link
Collaborator

eps1lon commented Mar 29, 2020

React version: 16.3-16.13

Steps To Reproduce

  1. Render function component with side-effects and without hooks in StrictMode
  2. Component only renders once

Link to code example: https://codesandbox.io/s/strictmode-w-and-wo-hooks-vgxvh

The current behavior

StrictMode only renders function components with hooks twice following #15074 (comment)

The expected behavior

I think making the distinction between components with and without hooks causes more confusion than it helps. Especially since the docs do not mention this. I amended past efforts to improve the StrictMode docs but since this has been ignored I'm not sure this helps. I would also work on making the behavior consistent across all function component types if this is possible.

@eps1lon eps1lon added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 29, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2020

Yeah I think it would be reasonable to flip it for all functions. Happy to take a PR doing that.

@gaearon gaearon added Type: Enhancement and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Mar 29, 2020
@gaearon gaearon changed the title Bug: Consistent StrictMode Call all functions twice in StrictMode Mar 29, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2020

since this has been ignored I'm not sure this helps

Would you like to volunteer maintaining the docs? Tbh we're a bit swamped by all the other work right now and this would be helpful.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 29, 2020

I think this proposed change makes sense in regular DEV usage, but wrt tests, it's probably worth discussing the reason we didn't do this in the first place:
#14639 (comment)

This change might be observable in a way that breaks existing tests (e.g. if I mock a module/component and then assert on the number of renders). Not sure how common this is, and also– I think this change might still be the right thing to do in a minor. Just seemed worth considering.

Just look at our own tests to see lots of special handling for double rendering and expecting yielded values.

That being said, maybe there aren't many pre-existing tests that are in strict mode, so maybe this is still the right trade off- but I expect we'll get some negative push back from this change.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 29, 2020

Would you like to volunteer maintaining the docs?

Sure. I can probably dedicate an hour or two per week.

we'll get some negative push back from this change.

I'm not so sure about it either. We had this only come up because we also publish a hook as a higher-order component and the user (and I) were confused why one would render twice while the other didn't.

However, I hope it's easier to justify that StrictMode renders twice than explaining why it only renders twice in certain situations. Maybe it's even better to have it appear like render-count is non-deterministic? 😄

@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2020

I think let's try to sync to WWW and see how much this breaks. I think there is an argument that if you're counting different method calls, we've already changed that for sCU recently, so there is a precedent.

We "break" tests that assert on console calls when we add new warnings, but that's a part of our policy. I think this is similar. Additionally, the number of renders isn't even deterministic in general. Our bailout heuristics can, and will, change too.

@jyash97
Copy link
Contributor

jyash97 commented Mar 30, 2020

Hey @gaearon @bvaughn just checking if it will be a good way to let developers know about the extra rendering in form of console warning when strict mode is enabled.

I have seen many developers ask the same question on Stackoverflow and on Github issues. How about a warning like this:

StrictMode will intentionally double-invoke render, constructor, etc to detect side effects. Read more [here](https://reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects) 

Thoughts ?

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2020

This will just be annoying noise IMO. If a mode produces more logs even when everything is good, people will turn it off to reduce the noise.

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2020

I have seen many developers ask the same question on Stackoverflow and on Github issues

Eventually there'll be enough answers for them to come up in results and people will stop asking. :-)

@jyash97
Copy link
Contributor

jyash97 commented Mar 30, 2020

This will just be annoying noise IMO. If a mode produces more logs even when everything is good, people will turn it off to reduce the noise.

Yeah I agree on this behavior, I did not think in this direction.

I have seen many developers ask the same question on Stackoverflow and on Github issues

Eventually there'll be enough answers for them to come up in results and people will stop asking. :-)

Haha true, doing the same by guiding people.

Thanks for the quick reply and sharing your thoughts :)

@ScottWager
Copy link

I've been using react for about a week and have just spent 3 frustrating hours trying to figure out why my app component was rendering twice, to finally find out it was React.StrictMode in my index.js file (when I removed my useReducer the issue went away so I thought it was that). I used 'create react app' to start my project so I didn't even know what StrictMode was because it was already set up for me.

It would save beginners like me countless hours if there was a comment at the top of the App.js file when you initiate 'create react app' with a message like jyash97 outlined above: "StrictMode will intentionally double-invoke render, constructor, etc to detect side effects. Read more here"

There are similar comments in the other files to help new people understand what is going on so I think it would really help.

Apart from this problem, I love react so thanks for the hard work!

@ScottWager
Copy link

@gaearon
Copy link
Collaborator

gaearon commented Apr 6, 2020

@ScottWager I understand your frustration but as I already said, this will be a default behavior (even without StrictMode) in the future so this wouldn't help beginners in longer term. What would help is learning that you shouldn't be "counting renders" because generally saying, React may render your component anytime. I expect that now that CRA includes StrictMode by default, more tutorials will mention that, solving the problem you described.

@gaearon
Copy link
Collaborator

gaearon commented Apr 6, 2020

That said I think it's reasonable to add a comment to CRA template in the meantime.

@ScottWager
Copy link

'What would help is learning that you shouldn't be "counting renders" because generally saying, React may render your component anytime.'

This is good advice, thank you!

@FgoPan
Copy link

FgoPan commented Apr 27, 2020

@eps1lon I found the same question as yours.
I had test several versions of React. But it is not still work well.When I delete the <React.StrictMode>.It's OK.
https://codesandbox.io/s/silly-breeze-7pu4i?file=/src/index.js

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.

6 participants