solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.05k stars 914 forks source link

createContext type: the Context return type makes it impossible to destructure without force typing #627

Closed ArthurClemens closed 3 years ago

ArthurClemens commented 3 years ago

Describe the bug A new context is created with function createContext that accepts an optional generic type. For example:

type TStore = [
  Accessor<number>,
  {
    increment(): void;
    decrement(): void;
  }
];

const CounterContext = createContext<TStore>();

The return type is a Context, typed as Context<T | undefined>.

This "maybe undefined" type is propagated through useContext and the consumer of the context, for example

const [count] = useCounter();

This raises a TS error, because the resulting type of useCounter() may be undefined, so the array destructuring does not work.

It is undesired to force type the referenced store in order to make this work..

To Reproduce The result can be seen in this CodeSandbox

At index.tsx, line 7:

const [count] = useCounter();

Hover over the wiggly line at count to inspect its type.

Expected behavior

createContext types are exported in signal.d.ts:

export declare function createContext<T>(): Context<T | undefined>;

This should be:

export declare function createContext<T = undefined>(): Context<T>;

Reproduction CodeSandbox

ryansolid commented 3 years ago

Hmm.. I see because technically if it misses it should be what is passed into createContext which could be undefined but also be something else. It should never miss completely. Yeah good point. This has come up before but interestingly enough no one suggested this to date. I think this sounds reasonable.

EDIT: On further look we use overloads. In the example provided it should be undefined because it could be. If you don't provide a default value then it can be undefined if it fails to find a provider.

export function createContext<T>(): Context<T | undefined>;
export function createContext<T>(defaultValue: T): Context<T>;

These are both correct in their given scenario.

lxsmnsyc commented 3 years ago

Context<T | undefined> is reasonable enough since useContext may return the defaultValue when the given context instance cannot be found. That, of course, will return undefined in your case since you never provided a value for defaultValue. But, the lack of default type for the generic makes sense.

if you have useCounter as an example, you can do

function useCounter() {
  const value = useContext(CounterContext);
  if (value) {
    return value;
  }
  throw new Error('Missing CounterContext');
}
ArthurClemens commented 3 years ago

If the spec says it could return undefined, it makes sense to handle it at app level.

I do suggest to update the documentation at https://www.solidjs.com/docs/latest#usecontext

lxsmnsyc commented 3 years ago

This function creates a new context object that can be used with useContext and provides the Provider control flow. Default Context is used when no Provider is found above in the hierarchy.

I believe that's a sufficient explanation to the runtime behavior, though the current docs lacks an example.

ryansolid commented 3 years ago

Ok I guess this one is a no action beyond clarifying documentation. This is some work being done to rework the wording.

Thanks for creating the issue.

DeusProx commented 2 years ago

@ryansolid

Sorry, for necroing this topic, but I don't want to create a new ticket just for voicing my own opinion and asking question. If you think otherwise please tell me and I create a new ticket. I agree that the current behaviour is correct, but if using typescript you will always have to write an undefined check like @lxsmnsyc proposed. To not have to rewrite the same code all the time I just made it generic:

export const useContextWhenDefined = <T>(context: Context<T | undefined>): T => {
  const provider = useContext(context)
  if(provider === undefined) {
    throw new Error(`Context should not be undefined`)
  }
  return provider
}

In my opinion this could be put into the library, because it reduces boilerplate and improves ergonomics, but I'm also fine with having this in my own projects due to it's handling errors and that should be user land. It's generic enough for that though.

I'm also very interested in understanding why you choose to create the createContext and it's function overloads like they currently are. For me there are only two use cases when it comes to data stores: A global and a scoped store. Providers & Contexts are clearly an awesome play to create scoped stores, but to use them right in typescript you always have to either check if the store is undefined/really is provided by a component above the tree or it forces me to set a defaultStore, which basically is a global store and atleast I don't want to have that ever. Why do you think it's a good idea to have both a global and a scoped store at the same time saved in your context?

I understand that using useContext everywhere is encouraging to form a standard, but I think that global and scoped stores are really two different use cases and that if someone wants a global store, the person should just create it in a module and export it. It is much clearer to other devs who read the code that the store is global, when not using useContext. E.g. when porting the official example for context/store from jsx to tsx you have to create an interface for the store otherwise count, increment and decrement are unknown. One needs to add something like the following:

type CounterStore = (Accessor<any> | {
  increment: () => void
  decrement: () => void 
})[]
const CounterContext = createContext<CounterStore>();

in counter.tsx and

  const counter = useCounter();
  if (counter === undefined) {
    throw new Error("CounterStore is not provided");
  }
  const [count, { increment, decrement }] = counter

in nested.tsx just so that the typing is right. For me that's not the most intuitiv way of doing things.

ryansolid commented 2 years ago

Yeah... I did in this case basically lift React's pattern verbatim. There is a pattern of using provider less context there. but you are right that in that case it is basically just a global store. I've never used it that way but I assumed there was a good reason for it. It would be interesting to see if it is being used that way anywhere. I do tend to use Context for everything and never use global stores so there is uniformity there but these sorts of TS headaches are annoying.

DeusProx commented 2 years ago

First thank you for answering.

Yeah... I did in this case basically lift React's pattern verbatim. There is a pattern of using provider less context there.

Makes a lot of sense to make solidjs as similiar to react to enable developers to easily switch. I haven't used React myself, but looked at it briefly before looking at svelte and solidjs. I also found some react tutorials now, which also check the store for undefined values.

It would be interesting to see if it is being used that way anywhere

I only found ones with interfaces, but I probably just looked at a very small percentage of examples. I questioned myself today whether or not I should use providers when my store is basically global to the app and that it would be easier to not even use Context due to the typing problems. It was easiest to just create a factory function for the store and use to create the stores/context. This also enables us to read the typings from the factory function directly for the creation of the context.

import { createContext } from 'solid-js'
import { createStore } from 'solid-js/store'

interface Entity {}

const createEntityStore = () => {
  const [ entities, setEntities ] = createStore<Entity[]>([])
  const store = {
    entities: () => entities,
    add: (entity: Entity) => setEntities(contents => [ entity, ...contents ]),
    clear: () => setEntities([]),
  }
  return store
}

// global store without Context
const contentStore = createEntityStore()

// context with default
const EntityContextWithDefault = createContext(createEntityStore())

// context from interface without default
interface EntityStoreInterface {
  entities: () => Entity[]
  add: (content: Entity) => void
  clear: () => void
}
const EntityContextFromInterface = createContext<EntityStoreInterface>()

// context from return type of factory without default
type EntityStoreFromReturnType = ReturnType<typeof createEntityStore>
const EntityContextFromReturnType = createContext<EntityStoreFromReturnType>()

// context from factory with generics
const createContextFromFactory = <T>(_: () => T) => createContext<T>()
const EntityContextFromFactory = createContextFromFactory(createEntityStore)

For the default use case of a scoped/non-global store the lib could even abstract every boilerplate away:

const createStoreProvider = <T = any>(factory: () => T) => {
  const Context = createContext<T>()
  const useStore = () => {
    const store = useContext(Context)
    if(store === undefined) {
      throw new Error(`Context should not be undefined`)
    }
    return store
  }
  const Provider = (props: JSX.DOMAttributes<unknown>) => (
     <Context.Provider value={factory()}>
       {props.children}
     </Context.Provider>
  )
  return { useStore, Provider }
}

export const {
  useStore: useEntityStore,
  Provider: EntityProvider,
} = createStoreProvider(createEntityStore)

Now I only need to write a store factory function and can directly import the rest where needed. But I guess one would have to know if that's thought too simple and people are doing much more complicated stuff.

mdynnl commented 2 years ago

useContext could throw if lookup resolves to undefined and warn if defaultValue was returned on dev mode with clearer error message of course. Currently, the error message depends on what the user does with the context value, mostly [] = use_ or {} = use_.

lxsmnsyc commented 2 years ago

I don't think it's recommended for useContext to just throw when the lookup fails. There are use cases where consuming context is optional (e.g. routing), there are others that would want to handle things differently if the value doesn't exist. I just think the API is okay right now, it just needs some proper documentation on how to handle these things correctly when working with TypeScript.