Fallback to local state when there's no Provider

[ehahn9]

Thanks so much for constate - wonderful, indeed!

I'm using it in a rather large project and finding that it is tedious to wrap every test with a Provider. React's use of a default context value in createContext helps with this normally because then useContext will get the default, even if no provider is in the tree. Alas, you can't do that with constate because there's no way use the hook outside of a component.

I'd love to get your thoughts on this... What I'm thinking of is something like this (admittedly big) api change:

// OLD
const counter = useContext(CounterContainer.Context);

// NEW
const counter = CounterContainer.use();

The use() function does the useContext subscription, but also knows how to call the
underlying hook instead if the context is empty. This makes writing tests really easy - just like React's createContext behavior when no provider is present.

There are lots of (mostly unfortunate) issues with this proposal, tho:

1. Since we use undefined in the context to signify the no-provider case, container hooks can never return undefined (seems pretty minor?). Easy to guard for that case.

2. Since use() has to call the hook when no provider is given, container hooks cannot have
   props anymore (seems pretty major).

3. It is a bit nasty to conditionally call the hook (use will call it if no provider is present, otherwise it won't). But this might not be a problem ITRW because you're either running real code (with a provider) or you're running tests (without a provider) and the render tree should be consistent across renders in each of those cases. React will complain if you get this wrong (like conditionally adding/removing providers, which seems super unlikely).

So it ends up like something like this:

function createContainer<V>(
  useValue: () => V,
  createMemoInputs?: (value: V) => any[]
) {
  const Context = createContext<V | undefined>(undefined);

  const Provider = (props: { children?: React.ReactNode }) => {
    const value = useValue();
    if (value === undefined) {
      throw new Error('Container hooks must return a value');
    }

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

  const use = () => {
    let value = useContext(Context);
    if (value === undefined) {
      warnNoProvider();
      value = useValue();
    }
    return value;
  };

  return {
    use,
    Provider
  };
}

[diegohaz]

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.

[ehahn9]

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!)

[diegohaz]

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. :)

[diegohaz]

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.

[diegohaz]

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) {
    ...
  }
}

[ehahn9]

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.

[bySabi]

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>);

[diegohaz]

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.

[ehahn9]

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!)

[diegohaz]

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.

[bySabi]

@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.

[timkindberg]

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.

[diegohaz]

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

[timkindberg]

@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.

[diegohaz]

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.

[diegohaz]

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

[timkindberg]

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.

[diegohaz]

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

[timkindberg]

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