Fix: Incorrect margin application

[andy-hook]

Related #31 (comment)

We noticed this issue in Radix since the dependencies in our lock files were changed. I'm pretty sure the underlying issue was exposed when the style dependency was added in this effect via react-style-singleton.

Accurate dependencies makes sense, but I think it was previously masking larger issues in consuming packages. As you can see in the original code, the offset query happens twice, once in the initial state setter and then again inside the useEffect. This wasn't an issue before as style-singleton wasn't updating the stored style so offset would always be calculated from the same initial state.

Here i've switched to updating the gap only once (and also when the prop is changed)

I'm not convinced this is the best approach but I've struggled to validate what the correct behaviour should be, or where an appropriate change should be made.

Feel free to point me in the right direction and i'll be happy to update accordingly or work with you to provide a better solution.

Many thanks in advance.

[peduarte]

Related: rainbow-me/rainbowkit#408

[theKashey]

This is correct change. I should not use useEffect to derive a value, but it's not clear why it fixes the problem.
UPD: because one cannot get the "gap" from the "mended" body element 🤦‍♂️

[andy-hook]

because one cannot get the "gap" from the "mended" body element

Yes, indeed. Any subsequent query on the body is going to take the already rendered gap and re-apply it. I do think this could be better handled in the code, though I wasn’t sure on the direction you'd prefer for a resolution.

It's also tricky for me to validate for regressions as I couldn't see tests, which is why I decided to go for this change as it simply respects the previous behaviour.



Do you feel this change is sufficient to resolve the issue for the time being?

[theKashey]

Yep. So in the very very beginning style-singleton was "immutable" for a reason (like this reason) - but in time it was forgotten and we are here.
I am just going to use it in the same "immutable" way. And add a dev-time check for overflow on the body - library should inform you if it cannot do something "right"

[lPadier]

In the react documentation:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

[Edit]
Currently, the code inside useMemo will only be run once, but it is not recommended to rely on this behavior.
I found this PR yesterday while investigating a layout shift in our application, and used a patch similiar to the one below using patch-package to correct it.
[/Edit]

  const [gap, setGap] = React.useState(getGapWidth(props.gapMode));
+  const prevGapMode = React.useRef(props.gapMode);
  React.useEffect(() => {
-    setGap(getGapWidth(props.gapMode));
+    if (prevGapMode.current !== props.gapMode) {
+      prevGapMode.current = props.gapMode;
+      setGap(getGapWidth(props.gapMode));
+    }
  }, [props.gapMode]);

Edit: Added some explanation as to where this patch comes from.

[theKashey]

Fix has been released as react-remove-scroll-bar@2.3.3
Propagating update to dependent packages....