search

mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.02k stars 1.24k forks source link

[charts][core] Replace useIsomorphicLayoutEffect of @react-spring/web for Base UI's helper #13431

Open oliviertassinari opened 2 months ago

oliviertassinari commented 2 months ago

Steps to reproduce

https://github.com/mui/mui-x/blob/ba8d2f2d08c0d1c1e48108004de7d01128d07ca6/packages/x-charts/src/hooks/useReducedMotion.ts#L1

looks suboptimal to me. useIsomorphicLayoutEffect should be using Base UI helper instead.

LukasTy commented 2 months ago

@oliviertassinari could you confirm the @mui/base helper you are referring to? 🤔 Maybe it is import useEnhancedEffect from '@mui/utils/useEnhancedEffect'; you were referring to? 🤔 In this case, it seems that the former might have a bit less coverage... 🙈 

const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;

vs

var isSSR = () => typeof window === "undefined" || !window.navigator || /ServerSideRendering|^Deno\//.test(window.navigator.userAgent);
var useIsomorphicLayoutEffect = isSSR() ? useEffect : useLayoutEffect;
michelengelen commented 2 months ago

is this still a valid issue then? 🤔

LukasTy commented 2 months ago

Probably a question to @mui/xcharts. 🤔 IMHO, using the existing effect doesn't hurt, especially if and when charts would drop material-ui dependency (then having a slight discrepancy between behaviors wouldn't have any effect). On the other hand, WDYT @michaldudak, would it make sense to extend the useEnhancedEffect with the additional checks that @react-spring/web is doing? 🤔

michaldudak commented 2 months ago

I haven't seen any issues regarding the current implementation, but indeed it seems that Deno has window defined during SSR. I'm OK with making it more reliable (I'd also call it useIsomorphicLayoutEffect, because useEnhancedEffect tells users exactly nothing, but that's another story ;).

oliviertassinari commented 2 months ago

could you confirm the @mui/base helper you are referring to?

@LukasTy I was thinking of useEnhancedEffect indeed. This helper still needs to be exposed from @base_ui/react/useEnhancedEffect but this issue will help us track progress / coordinate.

it seems that the former might have a bit less coverage

It feels like useEnhancedEffect is better because it's less bundle size, and I'm not aware of people complaining about its behavior.

using the existing effect doesn't hurt, especially if and when charts would drop material-ui dependency

It's part of why I opened this issue, the value I see: