search

gaearon / react-side-effect

Create components whose nested prop changes map to a global side effect
MIT License
1.22k stars 75 forks source link

Rewritten with FC #71

Closed magom001 closed 3 years ago

magom001 commented 3 years ago

Am I missing something, but can we not rewrite the whole thing with FC?

lourd commented 3 years ago

Hey @magom001, thanks for your initiative!

The difference between the implementation you've written and the current version is the behavior of useLayoutEffect and useEffect vs UNSAFE_componentWillMount. The componentWillMount lifecycle is executed as part of server-side rendering; useEffect and useLayoutEffect are not. This is by-design.

You might be able to get something equivalent by making initialization/instance registration part of the render codepath, like they do here in react-helmet-async.

Another good thing that library does is use context instead of static class properties to hold the state; that resolves concurrency issues when server-side-rendering. If this library's rewritten it should probably include that, too.

If there's a re-write we should probably also add more tests to cover SSR functionality and ordering behavior (see prior issues) to be confident that the rewrite isn't breaking anything.

Gonna close this PR for now, but definitely feel encouraged to re-open or start a new one if you're still interested in making this happen!