HenrikJoreteg / redux-bundler-react

Bindings for redux-bundler to React
37 stars 5 forks source link

WIP useConnect Hook #3

Open aulneau opened 6 years ago

aulneau commented 6 years ago

Hooks were recently announced with the release of an alpha of React 16.7.0. I wrote up a connect hook that I'm using in one of my apps that seems to work pretty well:

// Provider
import React, { Children } from 'react';
import { ReduxBundlerContext } from './context';

export const Provider = ({ store, children }) => (
  <ReduxBundlerContext.Provider value={{ store }}>
    {Children.only(children)}
  </ReduxBundlerContext.Provider>
);
// useConnect.js
import React, { useContext, useEffect, useState, useMemo } from 'react';
import { ReduxBundlerContext } from './context';
import produce from 'immer';
const spread = produce(Object.assign);

const cookArguments = (store, args) => {
  const keysToWatch = [];
  const actions = {};

  args.forEach(keyName => {
    if (keyName.slice(0, 6) === 'select') {
      keysToWatch.push(keyName);
      return;
    }

    if (keyName.slice(0, 2) === 'do') {
      actions[keyName] = (...args) => {
        if (store.action) {
          return store.action(keyName, args);
        }
        return store[keyName](...args);
      };
      return;
    }

    throw Error(`Can Not Connect: ${keyName}`);
  });

  return [keysToWatch, actions];
};

export const useConnect = (...args) => {
  const { store } = useContext(ReduxBundlerContext);
  const [keysToWatch, actions] = useMemo(() => cookArguments(store, args), [
    store,
    args
  ]);

  const [mappedState, setState] = useState(() => store.select(keysToWatch));

  const stateHandler = changes =>
    setState(currentState => {
      if (spread(currentState, changes) === currentState) {
        return null;
      }
      return spread(currentState, changes);
    });

  useEffect(() => {
    const unsubscribe = store.subscribeToSelectors(keysToWatch, stateHandler);
    return () => {
      unsubscribe();
    };
  }, []);

  return {
    ...actions,
    ...mappedState
  };
};
// context.js
import { createContext } from 'react';
export const ReduxBundlerContext = createContext(null);

Example usage:


const WalletPage = memo(({ ...rest }) => {
  const { signedIn, wallets, transactions } = useConnect(
    'selectSignedIn',
    'selectWallets',
    'selectTransactions'
  );

  return <Box>...</Box>

});

Any feedback would be appreciated!

Best t

abuinitski commented 6 years ago

Boom! Just updated my pet project for new goodies, went here to see what redux-bundler has to offer, and here you are πŸ‘ thanks!

At the same time I'm a bit concerned on useConnect implementation. Hooks concept is a bit fresh in my head so my thought process is still rather rusty. Please give me some credit on this.

Any action you dispatch will invoke listeners -> update state -> but, wouldn't trigger a re-render.

What would trigger a re-render – is a context provider changing – which should re-render your whole tree, recalculating all your selectors, and re-initializing effects, e.g. re-subscribing to the store.

But if you have all your components wrapped in React.memo, your re-rendering should stop since memo is supposed to only compare props. React doc clearly sais "If your function component renders the same result given the same props..." which tells me it does not try to look into memoized state.

So I'm completely lost at the moment and will try to play with your implementation and learn hooks a bit better :)

abuinitski commented 6 years ago

So right, it seems like using setState actually triggers a re-render for this specific component, same as for it's counterpart from a class world.

Which is good, but still one relevant comment remains: there is a lot of unnecessary state recomputations, as well as store re-subscription on each render. Everything works, but few bits of "laziness" and memoization are asking to be added.

abuinitski commented 6 years ago

And one more - useEffect will be called only after full tree will be mounted, rendered and finalized in all aspects. This creates a certain "dead" period when store can potentially receive updates that would slip through your callbacks. Maybe useMutationEffect will be more relevant here.

abuinitski commented 6 years ago

One major bug is spotted in your implementation – it comes from following difference:

  1. Class setState will merge previous state with passed object
  2. Hook setState will replace the whole state

subscribeToSelectors seems like only providing changed fields, so in your effect you should do

  const unsubscribe = store.subscribeToSelectors(
    keysToWatch, 
    changes => setState({ ...state, ...changes })
  );
aulneau commented 6 years ago

Thanks for your comments! Yeah I am kinda just shooting randomly still learning hooks and how to use them :)

And yeah, I found out that the state updater from useState only provides the diff, not the merged state.

I've made a couple modifications:

import produce from 'immer';

const spread = produce(Object.assign);

const stateHandler = newState => setState(spread(state, newState));
useMutationEffect(
    () => {
      const unsubscribe = store.subscribeToSelectors(keysToWatch, stateHandler);
      return () => unsubscribe();
    },
    [keysToWatch] // Only re-run the effect if keysToWatch changes
  );

could also be written like so (i believe)

useMutationEffect(
    () =>  store.subscribeToSelectors(keysToWatch, stateHandler),
    [keysToWatch] // Only re-run the effect if keysToWatch changes
  );
abuinitski commented 6 years ago

Yeah, I've tried this already, and I'm afraid that memoizing the subscribe effect does not work in this setup.

consider

const stateHandler = newState => setState(spread(state, newState));

useMutationEffect(
  () =>  store.subscribeToSelectors(keysToWatch, stateHandler),
  [keysToWatch] // Only re-run the effect if keysToWatch changes
);

if we don't re-run subscribeToSelectors each time, we always only have our initial version on stateHandler bound to store changes. And that initial version, in turn, has only initial version of state in it's context – not the latest one. So instead of doing setState(spread(currentState, stateUpdates)) it would do setState(spread(initialState, stateUpdates)).

I'm afraid we'll have to re-subscribe each time unless we find a better way.

aulneau commented 6 years ago

but if I understand it correctly: setState(spread(initialState, stateUpdates)), initialState is still only the state of the keys we're watching, so the stateUpdates will always be an update to one or many of those keys, right?

aulneau commented 6 years ago

and so, anytime setState runs, the state object will be up-to-date. I don't think it's stuck always as initialState

abuinitski commented 6 years ago

It will not be up to date I'm afraid :( Consider this sequence: initial call to useConnect -> selectState creates an initial state for us -> a closure stateHandler is created that captures that initial state -> an effect runs and bounds that stateHandler on store change

On the next run, stateHandler will not run again, meaning old version of stateHandler will be in the store, that still holds that initial state

Any store changes will merge new changes with that initial version of the state, resulting in a broken state.

abuinitski commented 6 years ago

Here is my current version:

import { useContext, useEffect, useState, useMemo } from 'react'

import ReduxBundlerContext from './ReduxBundlerContext'

export const useConnect = (...args) => {
  const { store } = useContext(ReduxBundlerContext)

  const [keysToWatch, actions] = useMemo(() => cookArguments(store, args), args)

  const [state, setState] = useState(() => store.select(keysToWatch))

  useEffect(() =>
    store.subscribeToSelectors(keysToWatch, changes => {
      setState({ ...state, ...changes })
    })
  )

  return {
    ...actions,
    ...state,
  }
}

function cookArguments(store, args) {
  const keysToWatch = []
  const actions = {}

  args.forEach(keyName => {
    if (keyName.slice(0, 6) === 'select') {
      keysToWatch.push(keyName)
      return
    }

    if (keyName.slice(0, 2) === 'do') {
      actions[keyName] = (...args) => {
        if (store.action) {
          return store.action(keyName, args)
        }
        return store[keyName](...args)
      }
      return
    }

    throw Error(`Can Not Connect: ${keyName}`)
  })

  return [keysToWatch, actions]
}
abuinitski commented 6 years ago

I'm still using useEffect instead of useMutationEffect – because on another thought (while I still can't formulate it properly) my brain thinks first variant is right.

aulneau commented 6 years ago

ooo I like how you've got it organized. I think I might not be running into the same issue you are. I can't seem to create a state change that doesn't update. Take this for example:

const [state, setState] = useState(() => store.select(keysToWatch)) 
// store.select(keysToWatch) = initialState, `state` is now our local 
// instance of global state for this instance of `useConnect`

  useEffect(() =>
    store.subscribeToSelectors(keysToWatch, changes => {
      setState({ ...state, ...changes }) 
     // because we're subscribed to any changes with these keys only, the `state` 
     // object will always be in sync until it unmounts, at which point the next time 
     // it mounts, initialState will pull from the global store
    })
  )

I'm not sure how to recreate a difference in state for the useConnect, because if any of the keys that are subscribed change, it should be reflected because the stateHandler is fed the latest version of the updated state, not the stale initialState.

aulneau commented 6 years ago

I have noticed a bug with:

  const [keysToWatch, actions] = useMemo(() => cookArguments(store, args), args)

for some unknown reason, this causes a bug where the state isn't updating like it should. I removed the useMemo from it and it's working fine now

abuinitski commented 6 years ago

I don't spot the bug yet, will update if do. At the same time here is a fix for effect memoization (trick: use functional state updates):

useEffect(
    () =>
      store.subscribeToSelectors(keysToWatch, changes => {
        setState(currentState => ({ ...currentState, ...changes }))
      }),
    [keysToWatch]
  )
abuinitski commented 5 years ago

So everyone could take a look at https://github.com/abuinitski/redux-bundler-hook

HenrikJoreteg commented 5 years ago

@abuinitski FYI, I tried to help let people know this exists: https://twitter.com/HenrikJoreteg/status/1097347950328475648