[React 18] Add full <StrictMode> support #7007

tkajtoch · 2023-07-27T22:25:26Z

Summary

Fix StrictMode and async rendering issues in EuiPortal, EuiWrappingPopover and EuiTourStep, and fix more flaky cypress tests found running on 6x CPU slowdown multiplier.

QA

Ensure unit tests and cypress tests are passing.

…date function in EuiWrappingPopover

…and EuiFlyout components

src/components/portal/portal.tsx

tkajtoch · 2023-07-27T22:34:48Z

src/components/portal/portal.tsx

    if (typeof window === 'undefined') return; // Prevent SSR errors

    const { insert } = this.props;

-    this.portalNode = document.createElement('div');
-    this.portalNode.dataset.euiportal = 'true';
+    const portalNode = document.createElement('div');


Elements have to be recreated on every mount (so twice in development in <StrictMode> environment) or React will do bizarre things with them trying to nest parent element in its children

Just want to make sure I'm understanding this change correctly - we moved all our instantiation logic from constructor to componentDidMount, and we found this issue in behavior due to strict mode remounting components twice in rapid succession. Does that sound right?

That's right, Cee!

tkajtoch · 2023-07-27T22:35:57Z

src/components/portal/portal.tsx

+    this.setThemeColor(portalNode);
+    this.updatePortalRef(portalNode);
+
+    this.setState({


Using state to keep portalNode reference up to date in case it changes when running in development and <StrictMode> and force re-render whenever it changes

It's not totally clear to me - when and why would portalNode change that would necessitate a rerender? We're creating portalNode ourselves here so it's not like the consumers can cause a rerender?

Does the interruption happen if a remount/rerender occurs in between lines 65 and 80? TBH, everything else in this PR makes sense to me, but this state change doesn't 😬

It's not that portalNode ever changes unexpectedly. We can't initialize portalNode in constructor anymore and there's no componentWillMount we could use in React 18. componentDidMount is fired after the initial render and if we just update a class property it won't trigger a rerender, we'd need to call this.forceUpdate() which is usually a red flag.

Before this change

✅ React 18, StrictMode disabled, development environment

constructor (this.portalNode is updated with div element reference)

render (this.portalNode is correct, so createPortal() is called)

componentDidMount (nothing to do here)

❌ React 18, StrictMode enabled, development environment

constructor (this.portalNode is updated with first div element reference)

constructor (this.portalNode is updated with second div element reference)

render (this.portalNode references second div element)

render (this.portalNode references second div element)

componentDidMount (nothing to do here)

componentWillUnmount (this.portalNode is removed from DOM 🚨)

componentDidMount (nothing to do here)

After this change

✅ React 18, StrictMode enabled, development environment

constructor (nothing to do here)

constructor (nothing to do here)

render (return null)

render (return null)

componentDidMount (this.portalNode is updated with div element reference)

componentWillUnmount (this.portalNode is removed from DOM)

componentDidMount (this.portalNode is updated again)

render (return createPortal())

render (return createPortal())

✅ React 18, StrictMode disabled, development environment

constructor (nothing to do here)

render (return null)

componentDidMount (this.portalNode is updated with div element reference)

render (return createPortal())

✅ React 18, StrictMode enabled, production environment

constructor (nothing to do here)

render (return null)

componentDidMount (this.portalNode is updated with div element reference)

render (return createPortal())

Thanks for taking the time to lay that all out, that helps a lot.

Just to be 100% clear though, In the After this change section, after every single componentDidMount step, technically another step should be listed that says something like "rerender due to this.state.portalNode update" - correct? Because basically the component mounts and then immediately rerenders, always?

That's right, even though it's probably not 100% optimal with the extra render, that's how react does things now anyway with hooks and everything.

Thanks for clarifying. Can we add an in-code comment documenting this behavior and why this.setThemeColor/this.updatePortalRef only need to be called on componentDidMount and not on componentDidUpdate even though state is updated?

What do you mean? setThemeColor and updatePortalRef are called whenever portalNode changes and that's the only thing that can trigger component updates. State is only used to store portalNode internally so render() can then use it. They could've been inlined to componentDidMount.

Ahh sorry, I think I'm following. For some reason I kept thinking it was possible for portalNode to change on prop change/update 🤦

It might be worth adding an inline code comment beforethis.setState() in any case noting that we're intentionally causing a rerender on mount. We have all that context here on this PR, but if we absolutely want to make sure it doesn't regress, an inline code comment will last longer / is easier to reference :)

That's a valid point. I added a comment explaining the reasoning behind setState in componentDidMount 👍🏻

tkajtoch · 2023-07-27T22:38:15Z

src/components/tour/tour_step.tsx

-  const [hasValidAnchor, setHasValidAnchor] = useState<boolean>(false);
-  const animationFrameId = useRef<number>();
-  const anchorNode = useRef<HTMLElement | null>(null);
+  const [anchorNode, setAnchorNode] = useState<HTMLElement | null>(null);


anchorNode may change when anchor prop gets updated. Using useState will trigger re-render for us.

Just to check my understanding, anchor is actually changing in production whenever tour steps move around the DOM, correct? If a tour step goes from step 1 to step 2, are we changing the popover anchor/contents and reusing the same popover instance, or are we rendering 2 separate popovers?

It just changes ref to state, the rest of the logic is literally the same

Right yeah, sorry, I was just trying to clarify my current understanding of production EuiTour behavior as it's been a while :) For what it's worth it looks like the answer is "separate popovers" / not reusing the same popover instance. So in production/reality it's unlikely (but not impossible!) that the anchor prop would update dynamically.

kibanamachine · 2023-07-27T22:49:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007_buildkite/

kibanamachine · 2023-07-27T22:52:01Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007/

tkajtoch · 2023-07-28T07:41:39Z

buildkite test this

kibanamachine · 2023-07-28T08:07:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007/

tkajtoch · 2023-07-28T09:15:57Z

Please ignore the failing buildkite job. Elasticmachine commit says it's building 88cf087 but it's actually checking out 6fc0fe2. Neither of them are correct

cee-chen · 2023-07-28T14:41:50Z

src/components/flyout/flyout.spec.tsx

-        'data-test-subj',
-        'euiFlyoutCloseButton'
-      );
+      cy.get('[data-test-subj="euiFlyoutCloseButton"]').should('be.focused');


Thanks for all these should('be.focused') changes - I'll definitely remember this for the future instead of using cy.focused()!

src/components/popover/wrapping_popover.tsx

src/components/tour/tour_step.tsx

cee-chen · 2023-07-28T15:09:51Z

src/components/tour/tour_step.tsx

-      animationFrameId.current = window.requestAnimationFrame(() => {
-        anchorNode.current = findElementBySelectorOrRef(anchor);
-        setHasValidAnchor(anchorNode.current ? true : false);
+      timeout = window.setTimeout(() => {


Do you mind leaving a code comment over why we chose setTimeout here over requestAnimationFrame? In the past Chandler would leave review feedback / have a preference to using requestAnimationFrame over setTimeout - it would be nice to document a conscious decision to use one or the other whenever we do

I added it here.

It is strongly recommended to use requestAnimationFrame whenever you execute a series of updates that cause layout repainting. In this case though we just wait for next event loop tick to try find anchor element in the DOM and the only difference between these two is that requestAnimationFrame may wait longer before executing as it's dependent on repainting speed (usually 60Hz / 16.6ms). This doesn't affect the chance of having a race-condition scenario in our code as it's not guaranteed for components to be rendered within a single frame / repaint.

Gotcha. That's probably be too much info to list in a single code comment but I'd really love it if we documented this in an internal wiki doc instead. I know there's multiple places in EUI where we alternate between requestAnimationFrame and setTimeout and it's honestly never clear why we made that decision. I think we need a singular place that recommends between one or the other, and we can link to that doc on a per-usage basis if necessary.

^ Sorry forgot to clarify, this is not a change request for this PR!

I updated my comment to explain it better here. I'll create an issue to write a doc on using setTimeout / requestAnimationFrame :)

You're awesome. Thanks Tomasz! Really appreciate us taking the extra time and effort to help our future selves (or future... dev descendants? 😅) not have to re-trip over some of the things we've had to figure out.

cee-chen · 2023-07-28T15:10:11Z

src/components/tour/tour_step.tsx

+  return anchorNode ? (
+    <EuiWrappingPopover button={anchorNode} {...popoverProps}>


This feels cleaner to me in addition to fixing a bug, so win/win all around! 🎉

src/components/portal/portal.tsx

src/components/tour/tour_step.tsx

kibanamachine · 2023-07-28T18:27:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007_buildkite/

cee-chen

My only remaining request is to document (in a code comment) the intentional decision to rerender after mount. That being said, that's already pretty thoroughly documented in this PR, so if you'd rather skip or wordsmith the comment that's totally up to you.

src/components/portal/portal.tsx

kibanamachine · 2023-07-28T18:54:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007/

kibanamachine · 2023-07-28T18:54:26Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007_buildkite/

…ount

cee-chen

Woohoo!! 🚢

kibanamachine · 2023-07-28T20:53:45Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007_buildkite/

kibanamachine · 2023-07-28T20:57:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7007/

elasticmachine · 2023-07-28T21:07:50Z

💚 Build Succeeded

Buildkite Build
Commit: 310312f

History

💚 Build #111 succeeded 5cf9385
💔 Build #107 failed ca40144
💔 Build #101 failed 88cf087
💔 Build #96 failed 88cf087

cc @tkajtoch

tkajtoch added 4 commits July 27, 2023 21:54

fix: support EuiPortal in strict mode

eff707f

fix: don't use useRef values in function component render path

a716934

fix: move button insert logic from componentDidMount to anchor ref up…

7728f56

…date function in EuiWrappingPopover

test: reduce cypress tests flakiness for EuiContextMenu, EuiDataGrid …

88cf087

…and EuiFlyout components

tkajtoch requested review from cee-chen and a team July 27, 2023 22:25

tkajtoch self-assigned this Jul 27, 2023

tkajtoch commented Jul 27, 2023

View reviewed changes

src/components/portal/portal.tsx Show resolved Hide resolved

tkajtoch commented Jul 27, 2023

View reviewed changes