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 Type Incompatibility #300

Closed
thatcort opened this issue Jan 5, 2021 · 11 comments
Closed

Context Type Incompatibility #300

thatcort opened this issue Jan 5, 2021 · 11 comments
Labels
typescript relating to typescript or types

Comments

@thatcort
Copy link

thatcort commented Jan 5, 2021

The types related to contexts seem to have some incompatibilities.

Using the example from the docs, I get the following error on the value attribute of CounterContext.Provider:

(JSX attribute) value: ({
    count: number;
} | {
    count?: undefined;
})[]
Type '(State<{ count: any; }> | { increment(): void; decrement(): void; })[]' is not assignable to type '({ count: number; } | { count?: undefined; })[]'.
  Type 'State<{ count: any; }> | { increment(): void; decrement(): void; }' is not assignable to type '{ count: number; } | { count?: undefined; }'.
    Type '{ increment(): void; decrement(): void; }' is not assignable to type '{ count: number; } | { count?: undefined; }'.
      Type '{ increment(): void; decrement(): void; }' has no properties in common with type '{ count?: undefined; }'.ts(2322)
signal.d.ts(75, 9): The expected type comes from property 'value' which is declared here on type 'IntrinsicAttributes & { value: ({ count: number; } | { count?: undefined; })[]; children: any; }'

The State<...> wrappers around the data seem to be interfering with their use in the value attribute. I think it's because it's hiding the properties of the object it wraps (but I don't really understand the types, so could be wrong).

What's interesting, is that if I simplify it to just the counter value, then it works fine:

export const CounterContext2 = createContext({ count: 0 });

export function CounterProvider2(props) {
  const [state, setState] = createState({ count: props.count || 0 });

  return (
    <CounterContext2.Provider value={state}>
      {props.children}
    </CounterContext2.Provider>
  );
}

For reference, here's the actual context provider I'm trying to get to work:

import type { Session, SupabaseClient } from '@supabase/supabase-js';
import { createClient } from '@supabase/supabase-js';
import {createContext, createState, onCleanup} from 'solid-js';
import type { Props } from '../types';

export interface SessionData {
    supabase: SupabaseClient;
    session: Session | null;
}

const supabase = createClient(import.meta.env.SNOWPACK_PUBLIC_SUPABASE_URL!, import.meta.env.SNOWPACK_PUBLIC_SUPABASE_ANON_KEY!);
export const SessionContext = createContext<SessionData>({supabase, session: supabase.auth.session()});

export function SessionProvider(props: Props) {
    const [state, setState] = createState({supabase, session: supabase.auth.session()});

    const {data: authListener, error} = supabase.auth.onAuthStateChange(async (event, session) => {
        console.log(`Auth state change: ${event}`);
        setState({session});
    });

    if (error) {
        throw error;
    }
    
    onCleanup(() => authListener?.unsubscribe());

    return (
        <SessionContext.Provider value={state}>
            {props.children}
        </SessionContext.Provider>
    );
}

And the error message:

(JSX attribute) value: SessionData
Type 'State<{ supabase: SupabaseClient; session: Session; }>' is not assignable to type 'SessionData'.
  Types of property 'supabase' are incompatible.
    Type 'State<SupabaseClient>' is missing the following properties from type 'SupabaseClient': supabaseUrl, supabaseKey, schema, restUrl, and 8 more.ts(2322)
signal.d.ts(75, 9): The expected type comes from property 'value' which is declared here on type 'IntrinsicAttributes & { value: SessionData; children: any; }'
@ryansolid
Copy link
Member

Hmm the example from the docs is TypeScript incorrectly assuming an array instead of a tuple I think. This example seems to work properly I think: https://codesandbox.io/s/counter-context-gur76.

I wonder if this is a matter of optional parameters being missing at initialization or something of that nature. State atleast is supposed to be exposing all the same properties through. Hmm.. any thoughts @amoutonbrady as I know you've had some experience playing around with context types.

@amoutonbrady
Copy link
Member

Yeah I see the issue. I never encountered this myself because I do it in a multiple stage process to be sure to get the proper type fed into the context. This is how I'd recommend approaching it generally (at least with typescript).

Here's how I'd rewrite the example above to ensure proper type;

import type { SupabaseClientOptions } from '@supabase/supabase-js';
import { createClient } from '@supabase/supabase-js';
import { Component, createContext, createState, onCleanup, splitProps } from 'solid-js';

interface Props {
    supabaseUrl: string;
    supabaseKey: string; 
    options?: SupabaseClientOptions;
}

function createSupaBaseStore({ supabaseUrl, supabaseKey, options }: Props) {
    const supabase = createClient(supabaseUrl, supabaseKey, options);
    const [state, setState] = createState({ supabase, session: supabase.auth.session() });

    const { data: authListener, error } = supabase.auth.onAuthStateChange((event, session) => {
        console.log(`Auth state change: ${event}`);
        setState({ session });
    });

    if (error) {
        throw error;
    }

    onCleanup(() => authListener?.unsubscribe());

    return state;
}


export const SessionContext = createContext<ReturnType<typeof createSupaBaseStore>>();

export const SessionProvider: Component<Props> = (props) => {
    const [internal, external] = splitProps(props, ['children'])
    const supaStore = createSupaBaseStore(external)

    return <SessionContext.Provider value={supaStore}>{internal.children}</SessionContext.Provider>
}

Essentially what we are doing is extracting the creation of the context value into its own function and retrieving its type to have a single source of truth.

The problem that I did solve on my own library is that you might end up with an empty context value on first render... This might be desirable for some case, this might not in other. What you can do is set an extra propriety in the sate to assert when the state is ready or not and use the <Show /> component to render the children when everything is ready.

As to whether this is a just me finding workaround to have proper type or not, I found this pattern of defining context to be very predictable and easy to scale as the context grow (I usually use it for global state, but this can be used for any kind of context, local or global).

I hope this can be of any help.

@thatcort
Copy link
Author

thatcort commented Jan 6, 2021

Thanks @amoutonbrady, that helps explain things. createContext<ReturnType<typeof createSupaBaseStore>> in your example seems to be equivalent to createContext<State<SessionData>> in mine. So really the issue is that the generic type on createContext is raw, but its provider expects to deal in State<?> objects.

Would it make sense to change the definition of Context so that the type of the provider's value is State?

export interface Context<T> {
    id: symbol;
    Provider: (props: {
        value: State<T>;  <---- Instead of just T
        children: any;
    }) => any;
    defaultValue?: T;
}

(I might be missing something...)

Or maybe it's a problem with the definition of State and it's supposed to work without modifications? That seems to be the idea of the state proxies, but I don't understand them well enough (it's all still magic to me).


Unrelated, I noticed definitions for getSuspenseContext with suspicious looking increment and decrement functions. Wondering if that's supposed to be there?

@ryansolid
Copy link
Member

Context has no expectation of State objects.. it can be anything really. Sometimes I use them like simple Svelte Stores and just pass a signal tuple straight in there. Sometimes I just expose an object of methods that return promises as a sort of service layer. I do see why this is perhaps unexpected. The problem is the proxy isn't quite the same as the underlying object. I do wonder if I can just make it look that way and make the internals stop complaining a different way. TypeScript is not my strength in the least.


Solid's Suspense is very little more than context with a promise counter managing a control flow. Transitions are a bit trickier but a granular reactive library that picks up rendering at any point in the tree it's really just rendering the branch while collecting promises and if the number is bigger than 0 don't insert it and render fallback instead while waiting for all the promises to resolve. As each promise resolves it updates this detached tree. Then when the counter hits 0, trigger the computation to insert those nodes in the DOM already rendered.

@thatcort
Copy link
Author

thatcort commented Jan 7, 2021

Thanks for all the explanations. I also spent a bit of time looking at the source code to understand better what's going on. The problem seems to arise when dealing with the recursive wrapping of object properties in State:

export type State<T> = {
  [P in keyof T]: T[P] extends object ? State<T[P]> : T[P];
}...

[P in keyof T]: T[P]; seems to work fine, but the compiler complains about [P in keyof T]: State<T[P]>. Maybe this is a Typescript bug and should be filed as an issue?

For now @amoutonbrady's suggestion of using the return type of a creation function will have to do.

@alexmarucci
Copy link
Contributor

alexmarucci commented Jan 7, 2021

Hey @thatcort ,

Yeah, it looks like State is failing to infer the correct typing so your State<SessionData> !== SessionData;

Two ways I can see this solved:

1. Don't pass the initial value to the context
export const SessionContext = createContext<State<SessionState>>();

2. Based on what @amoutonbrady has suggested, create the state and always pass that to the context:

const [sessionData] = createState({ supabase: createClient("a", "b") });

export const SessionContext = createContext(sessionData);

// ... then in your provider (after onAuthStateChange and cleanup functions)
return (
    <SessionContext.Provider value={sessionData}>
      {props.children}
    </SessionContext.Provider>
);

3. Use type casting (...but probably not the best option 🤷‍♂️)

return (
    <SessionContext.Provider value={state as SessionData}>
        {props.children}
    </SessionContext.Provider>
);

@thatcort
Copy link
Author

thatcort commented Jan 9, 2021

One additional thought I had on this topic: The definition of State says that object properties are State wrapper types and not the original types. This seems correct, but screws up usage of the State objects, since they are no longer compatible with the types they wrap. So we could fool the type system by adding the original property as well as the wrapped one.

This would change the definition to

export type State<T> = {
  [P in keyof T]: T[P] extends object ? State<T[P]> & T[P] : T[P]; // <--- Added "& T[P]"
} ...

Just an idea... Not sure if this could screw up other aspects of the type system that depend on only the wrapped definitions existing.

@alexmarucci
Copy link
Contributor

I think I found the underlying issue. Apparently when iterating over the props of an object, those marked as "protected" or "private" inside a class are ignored by the typing system therefore the "missing props" error.

Find the original discussion here: microsoft/TypeScript#13543

The proposed solution State<T[P]> & T[P] to circumvent this issue looks good to me though.

Reproduced it here Typescript Playground

type State<T> = {
  [P in keyof T]: T[P] extends object ? State<T[P]> : T[P];
  //[P in keyof T]: T[P] extends object ? State<T[P]> & T[P] : T[P]; // <--- Proposed fix
};

class A {
  protected a = '1';
  b = '2';
}

function expect<T>(s: T) {}

const state = {a: new A()} as State<{a: A}>;
expect<{a: A}>(state); // TS Error:  Property 'a' is missing in type 'State<A>' but required in type 'A'

@ryansolid
Copy link
Member

Hmm.. are you using classes for State? Just throwing it out there because classes work for State but not nested state as we basically want to avoid granular updates on certain built-ins (dates, regex, HTMLElement, and so on) so I used the same heuristic as MobX which seems to work. I do make mention in the docs of not using classes and only POJO's for state.

That aside I think that is probably a safe move. It might be misleading type-wise but it doesn't have that big of an impact I think. It does potentially push the issue further down the line but I guess less people would hit it. I'd love for any TypeScript experts to chime in. I will post on the discord to get some opinions. But otherwise I think we should be able to go with this.

@thatcort
Copy link
Author

@alexmarucci I don't think it's due to private/protected class members, since my examples at the top of the page used plain objects. It was the nested objects that caused problems

@ryansolid I agree this is just pushing the problem down the line, but will avoid type errors in code that otherwise works. I'm also not sure what a more correct solution is.

@alexmarucci
Copy link
Contributor

alexmarucci commented Jan 15, 2021

@thatcort Not directly, but supabase "createClient()" instantiates a "new SupabaseClient()" behind the scene, a class that has few protected properties

https://github.com/supabase/supabase-js/blob/02c1ddd84b7d221eb79ba2fc6d4c35e1eca95f17/src/SupabaseClient.ts#L31

@ryansolid ryansolid added the typescript relating to typescript or types label May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript relating to typescript or types
Projects
None yet
Development

No branches or pull requests

4 participants