chore: Replace useLayoutEffect during server rendering to reduce excessive warnings

[avinashbot]

• chore: Upgrade ESLint to 8.20 and unblock eslint plugins
• chore: Replace useLayoutEffect during server rendering to reduce excessive warnings

Description

Very low-hanging SSR fruit. SSRing or SSGing any components with a useLayoutEffect causes React to print out a massive warning per instance that crowds out the render logs, leading to less than ideal DX and making real issues harder to debug. This isn't going to be fixed on React's side, so the best place to fix this is on our side.

A pretty common solution to this is to create a custom hook that runs useEffect on the server and useLayoutEffect on the client, which this PR introduces. Also had to upgrade ESLint because one of the config options wasn't supported until recently (importNames).

I'm sure this'll inspire some healthy debate. We can discuss this as a team tomorrow :)

How has this been tested?

Added a unit test for the hook, as well as eslint checks to prevent regressions in this area.

Documentation changes

[Do the changes include any API documentation changes?]

• No.

Related Links

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

• Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
• Changes do not include unsupported browser features, see CONTRIBUTING.md.
• _Changes were manually tested for accessibility, see accessibility guidelines.

Security

• _If the code handles URLs: all URLs are validated through the checkSafeUrl function.

Testing

• Changes are covered with new/existing unit tests?
• Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Merging #79 (0cded47) into main (2d17bb2) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   92.29%   92.33%   +0.04%     
==========================================
  Files         532      533       +1     
  Lines       15460    15466       +6     
  Branches     4257     4258       +1     
==========================================
+ Hits        14269    14281      +12     
+ Misses       1106     1100       -6     
  Partials       85       85              

Impacted Files	Coverage Δ
src/button/icon-helper.tsx	100.00% <ø> (ø)
src/icon/internal.tsx	92.15% <ø> (+12.15%)	⬆️
src/app-layout/utils/use-focus-control.ts	100.00% <100.00%> (ø)
src/app-layout/visual-refresh/context.tsx	81.30% <100.00%> (ø)
src/app-layout/visual-refresh/layout.tsx	97.10% <100.00%> (ø)
src/area-chart/model/async-store.ts	100.00% <100.00%> (ø)
src/container/internal.tsx	91.89% <100.00%> (ø)
src/container/use-sticky-header.ts	83.67% <100.00%> (ø)
src/internal/components/dropdown/index.tsx	93.33% <100.00%> (ø)
src/internal/components/masked-input/index.tsx	100.00% <100.00%> (ø)
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d17bb2...0cded47. Read the comment docs.

[gethinwebster]

I'm not quite convinced about this - it feels like we're hiding away what are real warnings if we ever want to properly support SSR. It's potentially even useful for customers to see the warnings, to know that the components they're using aren't fully SSR-compliant?

As you say - a good topic for team discussion...

[gethinwebster]

I guess this would still not stop people from using React.useLayoutEffect? But hopefully that would get caught in PR as incorrect style anyway

[avinashbot]

Yeah, ESLint isn't super comprehensive here. I could probably throw in a no-restricted-properties for React.useLayoutEffect, but it's just linting, there are always ways around it. But this should catch it 99% of the time.

[avinashbot]

Some additional context to this fix:

Supporting SSR is different to removing useLayoutEffect. A component isn't "SSR-compliant" if useLayoutEffect is removed, it's a warning from React to check our assumptions that the state of a component looks good before the layout effect runs (we had this issue on the website today #76). We can't just switch them all to useEffect and say they're SSR-compliant. So if we ever want to say we're "testably" SSR-compliant, useLayoutEffect has nothing to do with it.

This solution isn't new either; you can see this being used in Carbon, Material UI, Ant Design (via rc-util), Fluent UI, and so on. It is annoying that this hack has essentially been standardized in the React world, but that's the way it is.

Of course, in an ideal world, we wouldn't be using useLayoutEffect at all. But there are places in the code where we feel it's better for a change to happen synchronously, usually to avoid flashes of incorrectly rendered content between state changes. As long as we have useLayoutEffect, we'll always have those annoying warnings being printed in every server-rendered framework (Gatsby, Next.js, Remix, etc) regardless of whether we support SSR or not, which isn't a nice look.

[gethinwebster]

I think what I particularly dislike about the solution is that it's non-transparently assigning a no-op function in SSR world (i.e. unless you know that useEffect won't be run, you might think that the code will run in both places). So maybe for transparency, assigning an explicit no-op function when window is undefined could be clearer to those reading the useIsomorphicLayoutEffect source.

[avinashbot]

Interesting, I hadn't thought of that. I want to stick with the "convention" of replacing effect types which everyone else uses just in case (if React starts considering hook ordering with server components or whatever), so that if React changes something in v19, they'll break everyone, not just us :P

EDIT: I'll also add a comment clarifying why we need this in the source.

[avinashbot]

Closing this PR for now following team discussion. We feel like we're hiding away potentially valid warnings and implying that our components are fully SSR-friendly, which they aren't.