Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[context] createContext function recommendation #60

Open
augustjk opened this issue Apr 4, 2024 · 3 comments
Open

[context] createContext function recommendation #60

augustjk opened this issue Apr 4, 2024 · 3 comments

Comments

@augustjk
Copy link

augustjk commented Apr 4, 2024

The current recommendation for createContext has

### `createContext` functions
It is recommended that TypeScript implementations provide a `createContext()` function which is used to create a `Context`. This function can just cast to a `Context`:
```ts
export const createContext = <ValueType>(key: unknown) =>
key as Context<typeof key, ValueType>;
```

This effectively makes typeof key to always be unknown.

Lit's implementation has

export function createContext<ValueType, K = unknown>(key: K) {
  return key as Context<K, ValueType>;
}

which allows taking an type argument for the context key. In order to take advantage of this though, both type arguments must be provided. If only ValueType is provided, K defaults to unknown as TypeScript does not do partial inference of type arguments.

Now in practice, I don't think having an explicit KeyType be provided to Context is quite necessary. The extraction of the ValueType does not require it:

```ts
export type ContextType<Key extends Context<unknown, unknown>> =
Key extends Context<unknown, infer ValueType> ? ValueType : never;
```

I'm a bit torn as it feels correct to allow explicitly typing the KeyType as in the Lit implementation, but if it's functionally moot, that creates confusion like lit/lit#4601

One issue with having KeyType be unknown is that it can look confusing. With unknown & {__context: ValueType}, it just looks like {__context: ValueType} in type intellisense.
e.g.

const key = Symbol('foo');
const foo = createContext<{foo: string}>(key);
//    ^? const foo: {
//           __context__: {
//               foo: string;
//           };
//       }
@Westbrook
Copy link
Collaborator

@augustjk did #59 fix this, or is there still more work that you'd like to see based on this?

@augustjk
Copy link
Author

augustjk commented Aug 5, 2024

@augustjk did #59 fix this, or is there still more work that you'd like to see based on this?

No, that PR only fixed inconsistencies and keeps the same createContext example mentioned in the original post of this issue.

Though I don't have a clear idea on what the change I'd like to see here. It's not detrimental as is since everything is functional, but it just feels less polished with hardcoded unknown types for the context key. But then Lit's implementation which does preserve the type of context key has its own downside of having to explicitly pass in 2 types to createContext. The author of lit/lit#4601 proposes a type registry so that's another possibility but would require agreeing on the module/namespace for the type.

@justinfagnani
Copy link
Member

Another option here is to drop the key type parameter. If keys are mostly used opaquely, then the value type is much more important. It's the type that producers and consumers need to use.

Yet another option is to wait for microsoft/TypeScript#26242 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants