React: Fix callback behavior in react@18

[tmeasday]

Issue: Our react docs E2E tests were failing.

The issue was that the setTimeout(0) approach of providing the STORY_RENDERED event in react@18 was not reliably firing after the story and all of its decorators had run.

What I did

Switched to a useLayoutEffect approach, as documented here: reactwg/react-18#5 (reply in thread)

An alternative would be to use a ref={} but that would require putting down an extra div which seems bad.

@valentinpalkovic @chantastic -- I think the two of you have more context here. I just diagnosed the issue and this seems to fix it.

How to test

Let's run the e2e-test-core job a bunch of times. If its fixed it should reliably pass!

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a9f61c7. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel=8 --max-parallel=15 -- --reset --optimized

---

Sent with 💌 from NxCloud.

[tmeasday]

I had to add this key={Math.random()} to ensure the callback is called one time per renderToDOM() call. I'm not sure if that exactly cancels out the useRef() above and we could actually simplify the whole thing.

I'm not sure I quite understood @sebmarkbage's comment about strict mode properly, because in my experience refs are not shared across multiple renders in strict mode -- for example in this sandbox, the count is logged as 1 two times, rather than "remembering": https://codesandbox.io/s/jovial-bartik-5llnzl?file=/src/App.js

I'd be inclined to simplify everything way down (removing the ref and the key) to just

useLayoutEffect(callback)

But I'd love to hear what others think @valentinpalkovic @chantastic (Chan is out this week so will need to wait until after that!)

[valentinpalkovic]

@tmeasday Could you tell me which comment by sebmarkbadge you mean? I cannot find the comment you are referring to.

My assumption is that a key property on the root element doesn‘t make much sense, because every time the React Dom render function is called a new component will be rendered. There shouldn‘t be any reference or connection to the previously rendered component.

[tmeasday]

Arggh, sorry @valentinpalkovic I meant to link the comment (updated now), it's in the code also, but here it is:
reactwg/react-18#5 (reply in thread)

So, the thought process on the PR as it currently stands is:

1. The useRef prevents the callback being triggered >1 times in strict mode for a single call to renderToDOM (and thus single render of <WithCallback>), following along the suggestion made in the comment.

2. However, if the WithCallback component is retained between calls to renderToDOM, that means the callback is never triggered again so we recreate the component each time, to guarantee exactly one call to the callback for each call to renderToDOM.

What I'm not sure of is if simply doing useLayoutEffect(callback) will (in practice or guarantee) end up having the same effect. I'm wondering if it will because in my experimentations, useRef doesn't actually seem to retain state across strict mode, in which case the ref check will have no effect.

[valentinpalkovic]

@tmeasday I think the codesandbox example isn't appropriate.

A better example to test strict mode and multiple executions of useEffect/useLayoutEffect is this one: https://codesandbox.io/s/autumn-platform-0rpc3f?file=/src/App.js

Although 0 is returned, the console.log clearly shows the iteration of the useRef value. A rerendering of course doesn't take place, because we are mutating ref.current, which doesn't trigger a rerendering. Strict Mode does not lead to a second rendering. Only effects are executed twice.

[valentinpalkovic]

@tmeasday

However, if the WithCallback component is retained between calls to renderToDOM

I couldn't find any information about this. Can you point me in the right direction? Is this just an assumption, or did you read that somewhere?

[tmeasday]

@valentinpalkovic what I mean is if you do:

root.render(<Component />)

// Then later
root.render(<Component />)

Then React does not reinstantiate Component, assuming that the variable here has not changed and is referentially equal (===). In particular, useEffect(..., []) will not run again. If you add a random key it will of course.

You can see it in action here: https://codesandbox.io/s/young-waterfall-zk8qeb?file=/src/index.js

[tmeasday]

As for where it is documented, I guess here? https://reactjs.org/docs/reconciliation.html#component-elements-of-the-same-type

[tmeasday]

One thing I should check is that the children of WithCallback are not also re-instantiated, which would break a bunch of things 🤔

[valentinpalkovic]

Very interesting. I didn't know, that React is even memoizing on the root level. I have understood this now! The question is whether unmountElement is called before renderElement is called once again with another Story. The key property shouldn't be necessary if that's the case. It is only necessary if root.render is called on the same instance. But if a new instance of root is provided, every time the renderElement function is called, the key property shouldn't be necessary. If this assumption is not the case, we should instead figure out why the timing of unmountElement isn't appropriate.

[tmeasday]

If this assumption is not the case, we should instead figure out why the timing of unmountElement isn't appropriate.

Ahh, so there are scenarios where you want to call unmount and other scenarios where you do not. For instance when changing args we don't want to remount the story/component, whereas when you change story, we do (that's what users expect in any case it seems, you could argue differently).

That's what we use the forceRemount flag to control.

[chantastic]

I'm sorry for the delay.

I wish I had something exciting to contribute but after reading the description and discussion this seems very reasonable.

I like that you update the basic method to the suggestion we got in the RWG thread. I don't specifically remember why that suggestion didn't make it in the first version but glad will be now.

I can't think of a better alternative to key={Math.random()}, if we want to guarantee that a re-render even if the component tree hasn't changed.

✅ from me

[tmeasday]

Note FTR I did add some unit tests to check this but as the monorepo is currently using react@16 and we don't have any strategy for unit testing different versions of React, I wasn't able to run them against React 18. This is something we want to think about soon.

https://github.com/storybookjs/storybook/tree/add-react-tests

[tmeasday]

Going to merge this as it seems to work (tested in one of our new examples (!)). @shilman we should patch this back to 6.5 I think.