rickyvetter / reductive

Redux in Reason
MIT License
409 stars 40 forks source link

ReductiveContext doesn't use the value provided by the ContextProvider #48

Closed albertoforni closed 4 years ago

albertoforni commented 4 years ago

I noticed that when using useSelector and useDispatch, it doesn't really matter the value passed by the <Store.Provider> since a valid store is passed in directly from the module when included.

So for example if we remove <ThunkedStore.Provider> from this example https://github.com/reasonml-community/reductive/blob/master/examples/react/reactEntry.re#L3 it still works.

I don't think this is the intended behaviour and I would suggest to create the context with a None value and unwrap it before using it, returning an error when it hasn't been passed in by the context.

This is what I'm thinking about

  let storeContext = React.createContext(None);

  module ContextProvider = {
    let make = React.Context.provider(storeContext);
    let makeProps =
        (
          ~value: option(Reductive.Store.t(Config.action, Config.state)),
          ~children,
          (),
        ) => {
      "value": value,
      "children": children,
    };
  };

  module Provider = {
    [@react.component]
    let make = (~children, ~value) => {
      <ContextProvider value={Some(value)}> children </ContextProvider>;
    };
  };

  let getStoreFromContext = () => {
    switch (React.useContext(storeContext)) {
    | None =>
      failwith(
        "could not find reactive context value; please ensure the component is wrapped in a <Provider>",
      )
    | Some(storeContext) => storeContext
    };
  };

  let useSelector = selector => {
    let storeFromContext = getStoreFromContext();
    ...

This would replicate the behaviour that react-redux has. And I think it's nice also in terms of testing because users can eventually pass in a test store value that has for instance mocked enhancers.

What do you think? If you are ok with that I can make a PR.

MargaretKrutikova commented 4 years ago

Yes, that would be great! Totally missed that, really good point about testing, and of course a PR is highly appreciated!

I guess we can also make getStoreFromContext into a hook called useStore? Just to follow the convention for custom hooks. Would be also great if you could add how to pass store in Provider into the README docs 😄

MargaretKrutikova commented 4 years ago

Released under v2.0.0, thanks a lot for your contributions!