Fallback to local state when there's no Provider about constate HOT 19 CLOSED

I can totally sympathize with the pushback. constate is wonderfully small and I agree bloating it would be bad. I'm totally fine w/ letting this thread die a nice peaceful death :-)

Although wrapping the consumer in a component for testing is really tedious, I agree about the testing case, primarily because testing the behavior of container (CounterContainer) is easily done by testing the underlying hook (useCounter). Testing a container is really testing constate, which isn't necessary. Things like https://github.com/mpeyper/react-hooks-testing-library make it so you needn't use a component wrapper/jsx in your hooks tests at all.

One final thought which might be worth considering before we close this topic (but again, totally fine to end the thread):

The React team addressed this exact pattern when they made createContext require a default value so that you could omit the <Provider>. Since constate is really 'hooks + context', the provider requirement seemed (to me) surprising. That's really a motivation for my OP.

...but I'm cool either way and appreciate the discussion (learned a lot!)

from constate.

Let's do that then. I think that would address #104 as well.

from constate.

So, in short, you're suggesting that, if there's no Provider, Constate should fallback to local state (as if useValue was used directly in the component), right?

That's an interesting idea and I'm open to it. I'd suggest an API like this:

import React from "react";
import createContainer from "constate";

function useCount({ initialCount = 0 } = {}) {
  const [count, setCount] = React.useState(initialCount);
  const increment = () => setCount(prevCount => prevCount + 1);
  return { count, increment };
}

// export a new useContainer method
// maybe deprecate Context
const { Context, Provider, useContainer } = createContainer(useCount);

function Component() {
  // useContainer(fallbackProps)
  // fallbackProps would be the hook props or just `true` to explicit say that it should fallback to local state.
  // fallbackProps would be used only when there's no Provider
  const { count, increment } = useContainer({ initialCount: 10 });
  ...
}

This way I guess there will be no breaking changes, only an addition of useContainer.

Also, we can't name it just use because it would break one of the rules of hooks: custom hooks names should match /^use[A-Z]/ so linters can identify them.

If the developer explicitly pass props to useContainer (or just true when the hook has no props), we could simply disable the no-provider warning since they probably want the fallback.

from constate.

Excellent! Really great feedback! Much more elegant and better in every way (esp retaining the ability to pass props!).

... oh, and are you okay with the limitation about a container hook not being allowed to return undefined?

Would you like to tackle this or can you wait awhile for a PR? (disclaimer: I'm pretty new at all this, so advanced apologies!)

from constate.

Yeah, this limitation isn't optimal since a hook could use only React.useEffect. Maybe we can convert undefined to null inside Provider and warn about it (instead of throwing an error):

if (value === undefined) {
  if (process.env.NODE_ENV !== "production") {
    console.warn("[constate] Container hooks must not return `undefined`. `null` will be used instead. You can explicitly return `null` to disable this warning.");
  }
  value = null;
}

Comparing to throwing the error, It would have less potential to break people's code, even though it's still a breaking change (one could expect context to return undefined, but not null).

About tackling this. I'm really busy these days with another project, so feel free to work on this. :)

from constate.

An alternative solution could be using a static context just to tell that Provider exists:

  const HasProviderContext = React.createContext(false);

  const Provider = props => {
    const value = useValue();
    // createMemoInputs won't change between renders
    const memoizedValue = createMemoInputs
      ? React.useMemo(() => value, createMemoInputs(value))
      : value;
    return (
      <Context.Provider value={memoizedValue}>
        <HasProviderContext.Provider value={true}>
          {props.children}
        </HasProviderContext.Provider>
      </Context.Provider>
    );
  };

  const useContainer = fallbackProps => {
    const hasProvider = React.useContext(HasProviderContext);
    if (!hasProvider) {
      ...
    }
  }

It looks hacky, but I guess that's totally valid use of context. And no breaking changes.

from constate.

Better (I guess):

const NO_PROVIDER = "CONSTATE_NO_PROVIDER"
const Context = createContext<V>(NO_PROVIDER as V);

const useContainer = fallbackProps => {
  const value = useContext(Context);
  if (value === NO_PROVIDER) {
    ...
  }
}

from constate.

Thanks. Had some time this afternoon so I've got a PR coming at you shortly. I ended up just checking:

if (value === defaultValue)

Since really that seemed to the intent (and interestingly, if Proxy's aren't available the default value
used to be {} which caused all kinds of interesting behaviors! So honestly, I think undefined might have fewer surprises.). I'm having trouble thinking of a use case for a value-less value hook inside a container, but maybe I'm not thinking clearly.

But feel free to noodle on this when you get the PR. I

I also added a bunch of tests for the useContainer cases and touched up the README.

from constate.

IMO adding useContainer for a test use-case only is bloated the code unnecessarily. Why you not just add a custom render function to your test code?

for Ex:

const render2 = children => render(<MyContainer.Provider>{children}</MyContainer.Provider>);

from constate.

If we're going to add this, it'll definitely not be for tests only. I can think of a few use cases where one would like to fallback to local state in production when using containers.

But this can be done in user land by wrapping useContext(Container.Context). This should be added to Constate only if it's a really common need.

from constate.

Being hooks + context is exactly why Constate requires a Provider. Hooks can't be called from outside a component, so we can't use them to set a default value. We simply take the hook from the consumer component and lift it up to Provider.

I updated the title to reflect better the enhancement I suggested. I still see value on this if this is a common need. But, after thinking better about it, right now I think it's not necessary as there are other easy work arounds. So I'm gonna close this for now.

@ehahn9 Thank you for raising this discussion and taking the time to play with the code.

from constate.

@ehahn9 your are absolute right in how tedious it is testing hooks with "react-hooks-testing-library" vs "react-testing-library".
Maybe you can take a look to hookleton and garfio that, in some cases, could be a constate alternative and don't relay on React Context.

from constate.

I am needing this recently. I want to provide common components to my team that can be used with or without a provider. If the provider is there the components make use of it, otherwise you are required to pass in the props to each component that they need.

from constate.

@timkindberg You could do this already, so I guess the only annoying thing right now is the warning, right?

from constate.

@diegohaz that's right, I just want it to stop spamming the console. I'm just worried my coworkers will notice and make a stink about it.

from constate.

We can maybe have a constate/no-warn entry point without the console warn or you can try to build constate with NODE_ENV as production, which would also prevent the warning.

from constate.

We can also just remove the warning and see if people will open issues because they're forgetting the Provider.

from constate.

I mean, the regular context doesn't do a warning (pretty sure, because that's what I ended up doing). So if folks are expected to use the core lib properly... I think it's fine to remove it from this lib.

from constate.

Looking for a PR, or will you be able to do it?

from constate.