Selecting multiple sub-states with <InjectStore />

[davej]

I'm converting a react-copy-write over to pullstate and it's going nicely.

I'm not using functional components everywhere so I can't use hooks everywhere I'd like yet. One place where it's not so nice is selecting multiple sub-states with <InjectStore />.

An example:

<InjectStore on={state => state.previewScreenshotOS}>
  {os => (
    <InjectStore on={state => state.apps[state.selectedApp]}>
      {(app: IApp) => (
        // ...
      )}
    </InjectStore>
  )}
</InjectStore>

That is with two sub states, you can imagine that it gets a bit unwieldy with 3 or 4. With react-copy-write I was able to do the following:

<Consumer
  select={[
    state => state.previewScreenshotOS,
    state => state.apps[state.selectedApp]
  ]}
>
  {(os, app) => (
    // ...
  )}
</Consumer>

I guess a possible workaround would be to not select sub-states in <InjectStore /> and just select the entire store.

<InjectStore>
  {state => {
    const {os, app: IApp} = state;
    return (
      // ...
    )
  }}
</InjectStore>

What do you think?

[davej]

Just to add to this. I've finished updating my app and it works a charm :)
It is noticeably slower than it was with though react-copy-write though, it's particularly noticeable when using controlled inputs.

I can do some deeper hunting if you like but my hunch is that this is because react-copy-write implemented a shouldComponentUpdate method and only re-rendered child components if the sub-state had changed. I'm looking into the useMemo hook now to see if I can regain some lost performance.

[lostpebble]

Great! Glad to hear it all works without any hitches. :)

Not so great to hear about the performance issue though. I'm wondering why that could be happening as I've set up a shallowEquals check on selected state to prevent re-renders. It could be that you're selecting too much state at once. For inputs, for example, you should just select the value you want to be editing, and only that component should only update when you change that value and nothing else in the tree.

If you can provide some example code I'll look into it! Otherwise, benchmark tests are definitely needed and something I'll look into soon.

On the first question though, the way to select multiple parts of the state is simply to return an object with the bits that you'd like to use:

Instead of doing this:

<InjectStore on={state => state.previewScreenshotOS}>
  {os => (
    <InjectStore on={state => state.apps[state.selectedApp]}>
      {(app: IApp) => (
        // ...
      )}
    </InjectStore>
  )}
</InjectStore>

Do this:

<InjectStore on={state => ({ os: state.previewScreenshotOS, app: state.apps[state.selectedApp] })}>
    {({ os, app }) => (
      <div>{/* use {os} and {app} */}</div>
    )}
</InjectStore>

Let me know if that works out for you!

[davej]

Oh wow, that solution above seems so obvious now :)

Another thought on performance. Even when we are reading state inside of the update method we are reading from the immer proxy object which is possibly a source for the performance issue? Might it make sense to pass the non-proxied state also?

// Currently
export const addIcon = (icon: UploadFile) =>
  update(draft => {
    const appId = draft.apps[draft.selectedApp || 0].id;
    draft.appIcons.push({
      appId,
      icon
    });
  });

// New API with unproxied state as second param???
export const addIcon = (icon: UploadFile) =>
  update((draft, state) => {
    const appId = state.apps[state.selectedApp].id;
    draft.appIcons.push({
      appId,
      icon
    });
  });

P.S.. I'm really liking typescript!

[lostpebble]

Ah, that could be related to it. I'll definitely look into that! I need to do some more reading on immer perhaps and see what is recommended to squeeze out the most performance - I have a feeling that what you've said about the proxy could be quite a big one.

Glad you're enjoying Typescript! It only gets better and better, then a little frustrating at times (when you get deep into generics and conditional types and try to be too smart for your own good), but I could never go back to regular Javascript now!

[davej]

Thanks for your help. I'll see if I can get to the bottom of it tomorrow.

I tried to use the react profiler but it wasn't giving me anything useful. This is what the flame graph looks like using the devtools performance tab, dispatchAction is from react-dom:

[lostpebble]

Cool, I'm gonna try investigate this as soon as possible too.

I'm just busy working on some async utilities for Pullstate now as I'd really like a simple way to take care of asynchronous state as well.

From the example you sent me earlier though, I think you should just make sure that you're always selecting the smallest possible state from your store.

This is generally not recommended (if state.apps[selectedApp] is a large object that you update inner values on - it sounds like it might be):

<InjectStore on={state => ({ os: state.previewScreenshotOS, app: state.apps[state.selectedApp] })}>
    {({ os, app }) => (
      <div>{/* use {os} and {app} */}</div>
    )}
</InjectStore>

Your entire app will be re-rendered if any value on state.previewScreenshotOS or state.apps[state.selectedApp] is changed.

Its better to use as precise a part that you want to view and update as possible in your tree, for example:

<InjectStore on={state => state.apps[state.selectedApp].name}>
    {(appName) => (
      <input value={appName} onChange={/* update state.apps[state.selectedApp].name */} />
    )}
</InjectStore>

This is where using hooks would help a lot to keep your components clean.

EDIT:

Oh, and btw, you can already access the non-proxied state on your store by using the _getState() function on it. Although, I am going to change this function name to getRawState() in an upcoming release. But if you'd like to start testing your hypothesis, you can use _getState() for the time being!

[davej]

Your entire app will be re-rendered if any value on state.previewScreenshotOS or state.apps[state.selectedApp] is changed.

Isn't this the case anyway (regardless of whether the value changes)? It seems that the component is re-rendered even if the substate's value hasn't changed.

For example 'was rendered: mac' gets repeatedly logged below whenever I type in an unconnected input box even though the value of state.previewScreenshotOS (a string) doesn't change

<InjectStore on={state => state.previewScreenshotOS}>
  {(os: IGlobalState['previewScreenshotOS']) => {
    console.log(`was rendered: ${os}`);
  }}
</InjectStore>

[lostpebble]

@davej think I just found a something which could give us a small performance gain - was not initialising the substate properly in useStoreState(), it was basically recalculating the substate on each render if it never actually changed.

Will push a new version asap.

[lostpebble]

For example 'was rendered: mac' gets repeatedly logged below whenever I type in an unconnected input box even though the value of state.previewScreenshotOS (a string) doesn't change

That should not be happening, unless that part of your React tree is wrapped in a component higher up which is reacting to the input you are typing in.

I'll try put something together on CodeSandBox just now to confirm.

[lostpebble]

@davej check out this https://codesandbox.io/s/oom973rv1q.

You'll see how each part of the react tree updates independently as per the console logs.

And I just released that little hotfix now as well - so maybe it could have been that which was causing issues too.

[davej]

That should not be happening, unless that part of your React tree is wrapped in a component higher up which is reacting to the input you are typing in.

Ah, ok, that makes sense. I had thought that it worked more like a shouldComponentUpdate method and it only changed when sub-state values had changed regardless of parent components.

I'll try the fix now and report back.

[lostpebble]

Ah, ok, that makes sense. I had thought that it worked more like a shouldComponentUpdate method and it only changed when sub-state values had changed regardless of parent components.

Interesting. I see your point. If there is a way to enact similar behaviour with hooks I'm all for it.

I see there is some information here: https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-shouldcomponentupdate

I'll definitely take a look and see. I was under the impression that children render regardless, but I see that this is not exactly the case.

For now, try wrapping your custom InjectStore component with React.memo(), as mentioned in that doc, and see if it makes any difference?

[davej]

I'll try optimizing the render tree and memo-izing to see if that fixes the issues.

If you want to look into it, here's a possible API that might work:

const App = () => useStoreStateMemo(
  UIStore, s => s.theme, 
  theme => (
    <div className={`app ${theme}`}>
      <button
        onClick={() => {
          UIStore.update(s => {
            s.theme.mode = theme.mode === EThemeMode.DARK ? EThemeMode.LIGHT : EThemeMode.DARK;
          });
        }}
      >
        Switch it up!
      </button>
    </div>
  )
);

I started implementing this with react-copy-write as useCopyWriteMemo here but the lib is no longer maintained and considered an experiment so I figured it best to go look for an alternative instead.

BTW: The fix didn't make any noticeable difference to performance timings in my use-case.

[davej]

Managed to get decent performance by implementing more shouldComponentUpdate and useMemo methods (this got me from ~110ms per keypress down to ~60ms per keypress). Finally, I debounced the global state updates from text input onChange events and now lag is no longer an issue.

I've still got the old version of the app on the master branch so I'll profile it against the old version and see if I can be more helpful in narrowing down the reason for the performance bottlenecks.

Thanks for your help! 🙂

[lostpebble]

No, thank you! :)

Really helps having you interacting with the library so rigorously and opening my eyes to things I probably wouldn't have caught for some time. You've been a great contributor.

Glad to hear the performance is getting better. ~60ms for a keypress still seems rather high to me? I guess it depends on many things. Though I'm looking forward to implementing some proper benchmarking soon to help locate any potential bottlenecks.

[davej]

Just checked the master branch and rendering is happening at about 70ms per key press so it's in the same ballpark as the optimised version of pullstate (without debouncing). I think the performance differences are accounted for by the fact that react-copy-write is more aggressive with preventing renders through shouldComponentUpdate.

It's relatively easy and perhaps more explicit to implement the same thing in user land with useMemo, React.memo and shouldComponentUpdate, so I don't neccesarily think this should be included in the lib. Perhaps if hooks could bail out of rendering (facebook/react#14110) more easily then it might be worthwhile.

[lostpebble]

@davej okay great.

I'm just curious though, is your app really large? Are there other things going on in the background with that keypress? 70ms still seems like a lot to me.

It's relatively easy and perhaps more explicit to implement the same thing in user land with useMemo, React.memo and shouldComponentUpdate, so I don't neccesarily think this should be included in the lib.

True. Something to consider though, if there are no downsides and it brings in nice performance benefits.

I'll keep an eye on that issue as well. The early bailout could be the best option.

[davej]

I'm just curious though, is your app really large? Are there other things going on in the background with that keypress? 70ms still seems like a lot to me.

The app isn't particularly large but the field updates other components, here's a screenshot:

The 'App Title' field updates text in two other components either side of it (and one other further down the page). I don't think that there was anything inherently slow in pullstate, I just misunderstood that it doesn't memo-ize components based on substate like react-copy-write. Regardless of the performance, debouncing the update to global state was a good idea, it was slow using the previous store lib too but not quite as slow, so I hadn't noticed.

True. Something to consider though, if there are no downsides and it brings in nice performance benefits.

Yes, I agree. Redux does it by default if you're using the connect() method and so does react-copy-write (the last lib I used).

The syntax that I suggested is prett ugly thoughh so probably better to wait for something nicer in React core:

useStoreStateMemo(
  UIStore, s => s.theme, 
  theme => (<div />)
)

[lostpebble]

Ah okay, makes a little more sense then. App looks great btw!

The syntax that I suggested is prett ugly thoughh so probably better to wait for something nicer in React core:

useStoreStateMemo(
  UIStore, s => s.theme, 
  theme => (<div />)
)

The useStoreState function should already be optimised, because it works on the useState hook and doesn't update the state unless the selected state is not shallowEquals(prev, next).

The one I was concerned about that you brought attention to is the <InjectStoreState/> which could be optimised further to not update even when parents update.

But the only way to find out is to start implementing some rigorous testing, which I'll try get to as soon as possible. The async utils are just about complete!

Yes, I agree. Redux does it by default if you're using the connect() method and so does react-copy-write (the last lib I used).

Cool, gonna look into using it by default - I think it makes sense to provide as much performance by default as possible.

[davej]

The useStoreState function should already be optimised, because it works on the useState hook and doesn't update the state unless the selected state is not shallowEquals(prev, next).

Yes but I believe it will also re-render each component below it, regardless of whether any of the child component's substate has changed. You can use React.useMemo() and React.memo() to get around this.

[lostpebble]

Hi @davej

I've made what I hope are some improvements to how Pullstate works internally. Including a fix to useStoreState() regarding listening to state (I think duplicate listeners, and in turn duplicate state updates, were being registered). As well as wrapping the children returned from <InjectStoreState> with useMemo(). Would love to know if it makes a difference to your app!

Also, the really big thing I've been working on these past few days was all to do with async actions. If you're using Promises in your app, would be great if you could try them out and let me know what you think of the interface and if it works out nicely for you (there's some docs for it at the bottom of the readme). I think I found a decent interface for it, but need to use it more now in my real-world app to make sure before releasing 1.0.0 eventually.

[davej]

Would love to know if it makes a difference to your app!

I tried the new version, it seems to have roughly the same timings for my particular use case. :)

As well as wrapping the children returned from <InjectStoreState> with useMemo()

I wonder is that a good idea? It means that <InjectStoreState> has different behaviour from useStoreState. Could be confusing particularly if a user is mixing standard component props and pullstate.

Also, the really big thing I've been working on these past few days was all to do with async actions.

Cool, I'll take a look.

I have code like this that seems to work fine. Should I consider moving it over to the new API:

async checkSecret(secret: string) {
  const { id } = this.props.match.params as any;
  if (id && id.length && secret && secret.length) {
    const app = await fetch(getAppJsonURL(id, secret))
      .then(x => x.json())
      .catch(x => null);
    update(draft => {
      draft.app.push(app)
    })
  }
}

[lostpebble]

I tried the new version, it seems to have roughly the same timings for my particular use case. :)

Okay - was hoping for some improvement, but glad to hear it's at least not regressing! lol

I wonder is that a good idea? It means that <InjectStoreState> has different behaviour from useStoreState. Could be confusing particularly if a user is mixing standard component props and pullstate.

Well this is pretty much the entire implementation:

export function InjectStoreState<S = any, SS = any>({
  store,
  on = s => s as any,
  children,
}: IPropsInjectStoreState<S, SS>): React.ReactElement {
  const state: SS = useStoreState(store, on);
  return useMemo(() => children(state), [state]);
}

Its just a convenience component that wraps useStoreState(). The useMemo() function will cause a re-render if the value of the state changes (cause of that second argument there) - so its kinda tied directly to useStoreState(), so I guess that should give it the same behaviour?

I have code like this that seems to work fine. Should I consider moving it over to the new API

So the big advantage of the async actions is keeping your view up to date with the execution state of your async function there- without putting too much onus on your stores for states like appStartedLoading, appFinishedLoading or appUpdating. So in your views you could do something like:

const [finished] = getAppAction.useBeckon({ secret, id });

return (
  <div>
    {finished ? <AppRender/> : `Loading app...`}
  </div>
);

It won't be necessary for all actions, its pretty use-case dependant. I just wanted an easy way to tie asynchronous updates to the view without filling my stores with too many flags. It also makes it easy to have your views trigger async things directly without doing things in componentDidMount() or useEffect() hooks.

Oh, and forgot to let you know what this the original state is passed to the update() function now as well, as the second argument.

[lostpebble]

You can see a (rather over-utilised) example of async actions here: https://codesandbox.io/s/84x92qq2k2?fontsize=14

[davej]

so its kinda tied directly to useStoreState(), so I guess that should give it the same behaviour?

Here's the difference in behaviour that I'm thinking about:

const Dock = ({ title }) => (
  <InjectStoreState on={state => ({ os: state.previewScreenshotOS })}>
    {({ os }) => <AppIcon title={title} os={os} />}
  </InjectStoreState>
);

In the example above,

• ✅If the os state parameter changes then it's a dependency of the the store so it will be updated.
• ❌ However, if the title prop that is passed into the component changes then it's not registered as dependency of useMemo(), so I believe that it won't be updated.

Not a big deal as long as it's documented but it could definitely be a gotcha since it differs from the behaviour if the hook and there's no obvious escape hatch to pass in more dependencies.

So the big advantage of the async actions is keeping your view up to date with the execution state of your async function there- without putting too much onus on your stores for states like appStartedLoading, appFinishedLoading or appUpdating.

That looks super useful. Look forward to trying it out.

[lostpebble]

Ah, thanks so much for the insight! That is rather concerning... and might be unexpected for a user (I know I would find it unintuitive and wonder what's going on). Just ran some tests and noticed that behaviour now for myself. Somehow thought that the inner children were still able to update, but it makes sense that they don't - useMemo() is a complete "do not pass" escape hatch it seems.

Hmm, kinda unsure which direction to take then. Will revert back to just the regular useState() for now until a better solution is found (if any).

That looks super useful. Look forward to trying it out.

I hope you find some use in it :) Btw, I'm just busy finalising some of the API still - but it's mostly concerning the value you return from an async action - I want to be a little more deliberate and opinionated about catching errors and success / failure states of that.

[davej]

Will revert back to just the regular useState() for now until a better solution is found (if any).

Yes, I think that works best. Best to leave it to the user if there's not a consistent and clear solution. It's possible to do it correctly using class-based components because you can use getDerivedStateFromProps (it gets called for a props update but not for a context update). There's not currently a way that I know of in functional components though.

I hope you find some use in it :) Btw, I'm just busy finalising some of the API still - but it's mostly concerning the value you return from an async action - I want to be a little more deliberate and opinionated about catching errors and success / failure states of that.

Cool, I'll keep you posted if I get a chance to test it.