Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useStore hook #733

Open
billyrrr opened this issue Oct 2, 2021 · 6 comments
Open

useStore hook #733

billyrrr opened this issue Oct 2, 2021 · 6 comments

Comments

@billyrrr
Copy link

billyrrr commented Oct 2, 2021

Is your feature request related to a problem? Please describe.

  1. useFluxible hook is not as expressive as connectToStore.
  2. There is a lack of documentation and clear way to "subscribe" to values in store when using hooks.
  3. The hook can be optimized to rerender exactly when StoreA out of [StoreA, StoreB] changes

Describe the solution you'd like
const locationStore = useStore('UserGpsLocationStore')
which would trigger a rerender on user Gps location change. The Gps location value is used in multiple components (similar as context)

  1. MapWithTracking
  2. A "Complex-Event-Processing" module to determine if the user is currently at a designated location
  3. A view to indicate where the user is currently at
    Before, the user's GPS location can be read from the context, but current implementation of fluxible-addons-react no longer supports legacy react Context API. It seems that the better way is to implement a useStore hook with a more fine-grained control over the data.

Describe alternatives you've considered
1.
Using current connectToStore HOC solves the problem (but less elegantly).
A wrapper has to be implemented on top of a hook component connectToStore(<HookBasedComponent />, [StoreA, StoreB])
Downside:

  • Does not take advantage of React hook's optimization. Component may be frequently updated on props change.
  • Looks more complicated than a hook-based component and more difficult for IDE to refactor and track type hints

Using current useFluxible hook
I am not very clear on the behaviour of this hook (whether it would trigger a rerender on Store change). Either way, it seems that a more fine-grained hook would be better.

I am considering implementing this hook, but hope to get some advice and feedback from the community first.

@redonkulus
Copy link
Contributor

@pablopalacios you want to take a look at this?

@pablopalacios
Copy link
Contributor

@billyrrr yeah, I completely understand what you are saying. Currently useFluxible is just a primitive hook that doesn't do much more than using the useContext to fetch fluxible context and provide it to the user/component.

There are only 2 use cases for it:

  1. read data from the store when you don't need to subscribe to it (eg. server data that is never updated in the browser)
  2. call executeAction

Creating the useStore is the natural next step. This section explain more or less how the flow should be: https://fluxible.io/api/components.html#accessing-stores. But the difference between this section and the real connecToStores is that the later also take the props in consideration to retrieve data from the store.

So, let's say, creating a very simple useStore that doesn't take component props in consideration should be simple:

function useStore(storeName, getStateFromStore) {
    const { getStore } = useFluxible().
    const store = getStore(storeName);
    const [state, setState] = useState(getStateFromStore(store));

    function updateState() {
        setState(getStateFromStore(store));
    }

    useEffect(() => {
        store.on('change', updateState);
        return () => store.removeListener('change', updateState);
    }, [store]);

    return state
}

Notes:

  • We need the getStateFromStore because, unfortunately, there is no interface for how to retrieve the store state;
  • I'm not so sure about the removeListener part (if it will work or not), but the point is, we also must unsubscribe from the store;
  • I'm also not sure about the useEffect dependencies here;

However, as soon as we add props in the game (let's say, to retrieve a todo item from the store given a todo id from the props), then things got a little bit more complicated. We also need to add the props in the useEffect dependency list. Then I would recommend you to read the issues that react-redux faced (https://react-redux.js.org/api/hooks#usage-warnings and https://kaihao.dev/posts/Stale-props-and-zombie-children-in-Redux).

But anyway, perhaps we could just start small and have no props support... @redonkulus what do you think?

@billyrrr if you can provide a PR I will gladly support you with reviews ;)

@billyrrr
Copy link
Author

billyrrr commented Oct 7, 2021

@pablopalacios Thank you for the detailed advice. I will start with the implementation of props-free useStore. Though, it seems that the user can specify the todo id by binding the id to the getStateFromStore function. For example:

function TodoItem({ todoId }) {
  const getStateFromStore = store => {
    return store.getTodoById(todoId);
  }
  const todo = useStore(TodoStore, getStateFromStore);
  return ...
}

If props-free useStore is to be released for production, I want to see if the following options could remedy this:

  1. pass the entire store as the return of the hook. (This may not work)
function TodoItem({ todoId }) {
  const store = useStore(TodoStore);
  const todo =  store.getTodoById(todoId);
  return ...
}

@pablopalacios has mentioned that:

We need the getStateFromStore because, unfortunately, there is no interface for how to retrieve the store state;

Is the lack of interface due to consistency issues? For example, getStateFromStore needs to invoked by fluxible rather than the user to avoid "user calling getStateFromStore on a weak reference". In which case, I would see why this can be a dead-end.

  1. Include in document a warning to state that getStateFromStore must be immutable and not to be updated on props change.
  2. Not releasing the prop-free hook for production.

I will continue with a PR based on your useStore draft, and at the same time we can keep the discussion going.

Thanks!

billyrrr pushed a commit to billyrrr/fluxible that referenced this issue Oct 7, 2021
…eady)

Co-authored-by: pablopalacios <pablo.palacios@holidaycheck.com>
@pablopalacios
Copy link
Contributor

@billyrrr regarding using props inside getStateFromStore, I'm pretty sure that it will not work (with the implementation that I proposed), since getStateFromStore will not be called when the props changes (only the function definition will be changed). But you can add a test case for it in your PR.

Regarding the lacking of interface, I mean, it would be cool if useStore could work like this:

const state = useStore('StoreName');

But since fluxible stores have no strict interface about how to retrieve their state, (eg. RouterStore has a bunch of getter methods that are specific to RouterStore while stores created with createReducerStore will have a getState method). Because of that, users must be responsible for retrieving the data:

const getStateFromStore = store => {
    return store.getState()
} 

const Component = () => {
    const state = useStore(Store, getStateFromStore);
    // ...
}

@billyrrr
Copy link
Author

I realized that there were some issues with a solution I proposed earlier. (most due to shallow prop comparison, and will not work) (It has since been deleted. Will post an amended version soon. Please ignore commit e415fe7 which will be rolled back soon)

@pablopalacios
Copy link
Contributor

@billyrrr I've created an example to demonstrate the stale props with the current connectToStores function. Since it's already an issue there, I think there are no blockers to already start using props with the useStore that you are doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants