Open billyrrr opened 2 years ago
@pablopalacios you want to take a look at this?
@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:
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:
getStateFromStore
because, unfortunately, there is no interface for how to retrieve the store state;removeListener
part (if it will work or not), but the point is, we also must unsubscribe from the store;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 ;)
@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:
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.
I will continue with a PR based on your useStore
draft, and at the same time we can keep the discussion going.
Thanks!
@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);
// ...
}
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)
@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.
Is your feature request related to a problem? Please describe.
useFluxible
hook is not as expressive asconnectToStore
.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)useStore
hook with a more fine-grained control over the data.Describe alternatives you've considered
Using current
connectToStore
HOC solves the problem (but less elegantly). A wrapper has to be implemented on top of a hook componentconnectToStore(<HookBasedComponent />, [StoreA, StoreB])
Downside: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.