react-redux icon indicating copy to clipboard operation
react-redux copied to clipboard

Enable forcing useLayoutEffect when using alternate renderers

Open speakingcode opened this issue 4 years ago • 6 comments

useLayoutEffect does not work with server-side rendering, and so a warning is displayed if it is used under Node. To get around this warning, useIsomorphicLayoutEffect tries to detect if the environment is not a browser (via the absence of window.document.createElement, etc) and falls back to useEffect if the check fails. Unfortunately, this may cause issues for alternate renderers that run in Node but are not using server-side rendering, such as react-blessed or Ink, and rely on useLayoutEffect to ensure store subscriptions happen at the appropriate phase of rendering.

This PR makes available a function forceUseLayoutEffect exported from alternate-renderers. A client of the react-redux library can call this to force use of useLayoutEffect in any environment.

import { forceUseLayoutEffect } from 'react-redux/alternate-renderers'
//...

forceUseLayoutEffect()

Implementation notes:

  1. Because calling forceUseLayoutEffect() is dynamic and will happen after all of the module files are evaluated (thanks to ES module imports), a getUseIsomorphicLayoutEffect dynamic getter is provided and existing imports and calls to useIsomorphicLayoutEffect are replaced with calls to the getter
  2. Dynamic importing (import(...)) is enabled via a babel.config.json file, as dynamic importing is necessary to mimic different import-time environment conditions for testing

speakingcode avatar Jan 28 '21 23:01 speakingcode

Deploy preview for react-redux-docs ready!

Built with commit c5925cff8772c5b197ee722717f0c146bbeb81ba

https://deploy-preview-1685--react-redux-docs.netlify.app

netlify[bot] avatar Jan 28 '21 23:01 netlify[bot]

Noticing that in TravisCI a test fails with

TypeError: _vm(...).SyntheticModule is not a constructor

I had this issue locally before providing a babel.config.json to enable dynamic imports. Not sure how to make TravisCI pick this up, or does it need to be configured in a different way (package.json, etc)?

speakingcode avatar Jan 28 '21 23:01 speakingcode

This is impressively causing Node to segfault, so is there some looping going on in the tests? That's usually the cause of those kinds of hard crashes.

@markerikson Did you want to take a look at this? I consider this more "your" code, since you implemented it originally IIRC.

timdorr avatar Feb 01 '21 21:02 timdorr

Yeah, I asked for this PR, and i do want to look it over. I've just got a lot going on atm and don't have the mental capacity to do so right now. Will try to, Soon (TM) :)

markerikson avatar Feb 02 '21 03:02 markerikson

It is strange that it's seg faulting, before the error was TypeError: _vm(...).SyntheticModule is not a constructor (which I also experienced locally when babel-plugin-dynamic-import-node was not ini the env:test config in .babelrc). There are no loops or circular logic, and the additional test coverage for forceUseEffect is structurally identical to forceUseLayoutEffect. That said upon looking I noticed I forgot to export forceUseEffect from alternate-renderers, and additionally the @babel/plugin-syntax-dynamic-import plugins entry is not necessary, only the babel-plugin-dynamic-import-node entry in env.test.plugins so I'll push those changes

speakingcode avatar Feb 02 '21 19:02 speakingcode

My inclination here is that we don't want to allow switching between useLayoutEffect and useEffect after the app has started up. I don't see any benefit to doing so, and it feels potentially confusing. Also, right now, we're adding in a couple extra function calls every time even though the odds of this ever changing are practically zero.

Problem is, I'm not sure how to restrict that. If we call the getter before the hook is defined, then the file probably gets imported and loaded well before your entry point has a chance to import and call forceUseLayoutEffect(), and the call sorta ends up being useless because we already got a handle to useEffect before you could override that.

I suppose we could add yet another ref to store that hook reference, but it feels like a waste.

markerikson avatar Mar 22 '21 23:03 markerikson

Heh, this never did get merged. I believe it's obsolete as of v8 because we switched to useSyncExternalStore.

markerikson avatar Jun 11 '23 16:06 markerikson