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

Double-invoking the render function in StrictMode is somewhat too deterministic #15065

Closed
dai-shi opened this issue Mar 8, 2019 · 13 comments
Closed

Comments

@dai-shi
Copy link
Contributor

dai-shi commented Mar 8, 2019

Do you want to request a feature or report a bug?

feature in the development mode

What is the current behavior?

Inside <StrictMode>, the render function is invoked twice, but always the second one is committed.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://codesandbox.io/s/k938koy8mv
I made a tiny change in @gaearon 's code.

  // This may not work in concurrent mode.
  savedCallback.current = callback;

What is the expected behavior?

Developers can notice the wrong code (unintentionally mutating refs in the render function).

One idea is to occasionally (ex. Math.random() < 0.25) commit the first result from double-invoked render function.
The other could be to introduce <StrictStrictMode> which is to triple-invoke the render function and commit the second one.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.8

@dai-shi
Copy link
Contributor Author

dai-shi commented Mar 8, 2019

A note from my experience: it took me a week to understand it.
Ref: https://twitter.com/dai_shi/status/1096947972871319553

@budarin
Copy link

budarin commented Apr 24, 2019

I have the same bug - useReducer was called twice.
For a long time I have thought that it's been problem with recreating reducer but found this issue and when have removed strict mode - the bug is disappeared

@dai-shi
Copy link
Contributor Author

dai-shi commented Apr 24, 2019

Hi @budarin
Double invoking in strict mode is intentional, not a bug.
This issue is about that just double invoking might not be enough.

@budarin
Copy link

budarin commented Apr 24, 2019

but there is no a description of this "feature"

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 24, 2019

I guess this is more of a docs issue. useMemo also runs twice in strict mode if the dependencies change.

@budarin
Copy link

budarin commented Apr 24, 2019

useMemo behaviour is described exhaustively unlike strict mode

@eps1lon
Copy link
Collaborator

eps1lon commented Apr 24, 2019

useMemo behaviour is described exhaustively unlike strict mode

I didn't find any mention in the docs that the factory in useMemo is invoked twice in strict mode when dependencies change.

@dai-shi
Copy link
Contributor Author

dai-shi commented Oct 17, 2019

@Baloche

#16956 (comment)

A little bit off topic but I need some clarification : In every implementation of useDynamicCallback that I see, a useEffect or a custom useIsomorphicLayoutEffect is used, but why ? What is the point of updating the ref inside a use*Effect instead of just doing it outside like the following ?

function useDynamicCallback(callback) {
  const ref = useRef()
  ref.current = callback
  return useCallback((...args) => ref.current.apply(this, args), [])
}

Indeed, it's an off topic, so let me comment it here.

Usually, it works like this:

render1 callback='abc'
commit1 callback='abc'
render2 callback='def'
commit2 callback='def'

However, sometimes commits can be skipped.

render1 callback='abc'
commit1 callback='abc'
render2 callback='def'
render3 callback='ghi'
commit3 callback='ghi'

And, there can be time gap between render2 and render3.
During the period, the callback should be 'abc', but if you don't use effect it will be 'def'.

@Baloche
Copy link

Baloche commented Oct 17, 2019

Why should it be 'abc' ? I'm actually expecting it to have the most recent value.

@dai-shi
Copy link
Contributor Author

dai-shi commented Oct 17, 2019

Because 'def' is thrown away by React. In that situation, the most recent value at that point is 'abc'.

Note: This is probably a hypothetical discussion for upcoming concurrent mode.

@dai-shi
Copy link
Contributor Author

dai-shi commented Nov 4, 2019

It is hard to reproduce the throw-away behavior intentionally.
So far, I've created this.
https://codesandbox.io/s/eager-ritchie-2qv63

@dai-shi
Copy link
Contributor Author

dai-shi commented Apr 6, 2020

I'd take this down. Double-invoking behavior is already confusing people and it's not a good idea to go with more weird behavior.

It would be the best to handle this issue with eslint rules/suggestions.

@dai-shi dai-shi closed this as completed Apr 6, 2020
@dai-shi
Copy link
Contributor Author

dai-shi commented Apr 8, 2020

#18545 looks like related to the concern in this issue.

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

No branches or pull requests

5 participants