search

ioof-holdings / redux-dynostore

These libraries provide tools for building dynamic Redux stores.
BSD 3-Clause "New" or "Revised" License
122 stars 15 forks source link

Fix/cross component update warning #347

Closed mpeyper closed 3 years ago

mpeyper commented 4 years ago
Q A
Fixed Issues? Fixes #234
Documentation only PR
Patch: Bug Fix?
Minor: New Feature?
Major: Breaking Change?
Tests Added + Pass? Yes

Moved the store and component enhancement of the "dynamic target" chain into useEffect of dynamic, preventing the attachReducer call (and any other enhancers that trigger a store update`) from updating the store in the main render path.

This change has an assumption that is is ok to render null for a single render until the store has been updated. Once the effect has triggered, the enhanced component will be created and set into state where it can be safely rendered in the subsequent renders. I think this is ok, but it is a change from the existing functionality, although it may improve a previously raised issues around concurrent mode and render tearing (where some of the rendering uses a particular instance of state and the another part uses a different instance of state, potentially creating inconsistent output).

I'm not sure if this should be a breaking change, in case anyone is relying on this behaviour, a minor change, because we may have introduced a new feature to prevent tearing, or a minor change, because we're fixing a warning about a potential bugs.

I'm leaning towards a major version, but I'm also open to being convinced it only a minor or patch version.

mpeyper commented 4 years ago

This has been published as version 3.1.1-alpha.0 (for all packages) for anyone that want's to try it out. Please let me know if you have any issues.

Ethorsen commented 4 years ago

Hey @mpeyper,

Just tested 3.1.1-alpha.0 with React 16.13.1.

So far so good, I'm seeing no difference at all in our application which heavily rely on dynostore/subspaces. Warning from react is gone.

I will keep pushing the tests but at this point I'm pretty confident I won't find any new/modified behavior. We will probably commit to use you alpha release until it makes its way into an official release.

Thank you very much for your efforts

mpeyper commented 3 years ago

Thanks for the feedback @Ethorsen. Assuming this is still working fine for you (please let me know otherwise), I'll get this into a release soon.

mpeyper commented 3 years ago

This has (finally) been released in v3.1.1

jlowcs commented 3 years ago

it seems like this fix is causing regressions when using SSR: https://github.com/ioof-holdings/redux-dynostore/issues/473