davidgilbertson/react-recollect

Collected components re-render when an unused property is created

Kycermann opened this issue · 3 comments

Recollect version
5.10

Describe the bug

initStore({
  data: {accessedByA: 100}
});

const A = collect(({store}) => { ... accesses store.data.accessedByA ... });

const B = collect(({store}) => { ... creates & modifies store.data.createdByB ... });

const App = collect(({ store }) => {
  return [<A/>, <B/>];
});

Creating store.data.createdByB in B causes A to be re-rendered. This should not be the case because A never accesses the property.

Modifying store.data.createdByB in B does not cause A to be re-rendered.

Here is a demo.

Thanks for the input and the demo!

This is by design. A component that subscribes to changes in store.data.accessedByA is also subscribed to changes in store.data. Imagine if B did store.data = 'Oh no!', this would change the data used by A, even though it didn't change store.data.accessedByA directly.

Creating a new property on an object is considered a modification of the object, since a component could listen to if (Object.keys(store.data).length) /* do something */. Hence, if you change the number of keys on an object (or array length, or map or set size), it counts as modifying the object.

Check out When will my components be re-rendered? in the readme and this test: https://github.com/davidgilbertson/react-recollect/blob/master/tests/integration/updating.test.tsx, which includes the same logic as your demo.

I think there is potentially room for improvement here, since as you rightly point out, this can cause renders that aren't necessary. But it's a balancing act: every extra check when reading/writing adds a tiny bit of runtime overhead, and increases bundle size. And adding new props to the store is probably not something that's done with great frequency. And the renders caused by this aren't actual DOM updates, React will bail early because the VDOM won't have changed, so the impact should generally be negligible.

I'm open to suggestions but for now I'm happy with this balance.

Thank you for the information!

That makes sense, I hadn't thought about cases like if (Object.keys(store.data).length) /* do something */.

My original issue, an unnecessary re-render, can be fixed by wrapping the created property in an object, i.e. store.data.extra: {createdByB}, where store.data.extra already exists.

Oh there's so many weird edge cases :)

Your solution sounds good. I might add that to the readme.

Also, if you know in advance what props you'll be using, you can run this before mounting your components to minimise updates later. Same idea as 'initial state' in Redux reducers.

initStore({
  data: {
    usedByA: null,
    usedByB: null,
  },
});