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

Full TypeScript support for $ #1

Closed
luisherranz opened this issue Nov 28, 2022 · 8 comments
Closed

Full TypeScript support for $ #1

luisherranz opened this issue Nov 28, 2022 · 8 comments

Comments

@luisherranz
Copy link

luisherranz commented Nov 28, 2022

I have been trying to make it work but have not yet succeeded. It's not clear to me that it's possible.

In principle, going through the object properties and adding a new property with $ seems possible since TypeScript 4.3:

export type DeepSignal<T extends object> = {
  [P in keyof T & string as `$${P}`]: T[P] extends object
    ? Signal<DeepSignal<T[P]>>
    : Signal<T[P]>;
} & {
  [P in keyof T]: T[P] extends object ? DeepSignal<T[P]> : T[P];
};

But there is a problem:

  • If $ is optional, TypeScript assumes that store.$prop may not exist: playground

    // `$` is optional
    export type DeepSignal<T extends object> = {
      [P in keyof T & string as `$${P}`]?: T[P] extends object
        ? Signal<DeepSignal<T[P]>>
        : Signal<T[P]>;
    } & {
      [P in keyof T]: T[P] extends object ? DeepSignal<T[P]> : T[P];
    };
    
    const state = {
      a: 1,
      b: {
        c: "2",
      },
    };
    
    const store = deepSignal(state);
    
    store.$a; // works fine
    store.b.$c; // works fine
    store.$b.value.c; // doesn't work
    
    store.b = { c: "3" }; // works fine
  • If $ is not optional, you cannot mutate the object without adding properties starting with $: playground

    // `$` is not optional
    export type DeepSignal<T extends object> = {
      [P in keyof T & string as `$${P}`]: T[P] extends object
        ? Signal<DeepSignal<T[P]>>
        : Signal<T[P]>;
    } & {
      [P in keyof T]: T[P] extends object ? DeepSignal<T[P]> : T[P];
    };
    
    const state = {
      a: 1,
      b: {
        c: "2",
      },
    };
    
    const store = deepSignal(state);
    
    store.$a; // works fine
    store.b.$c; // works fine
    store.$b.value.c; // works fine
    
    store.b = { c: "3" }; // doesn't work

Are you familiar with this problem? Any ideas on how to fix it?

By the way, I tested your library and it looks like you are just adding $ to the getters, aren't you? Is it for some reason? What is your idea regarding TypeScript?

I've recreated the example you have in the README in Stackblitz, and added a few $ to test this. I'm not sure if I'm doing it right.

@luisherranz
Copy link
Author

I wonder if we need to wait for something like this, or if there is a workaround that we can use today.

@luisherranz
Copy link
Author

luisherranz commented Nov 28, 2022

By the way, the two APIs that could work are (as far as I know):

  • Use a function to get the signals in TS (not required in JS)

    store.$b.value.c; // doesn't in TS, but works in JS
    signal(store, "b").value.c; // works fine in both

    You've tested that, haven't you?

  • Use a setter to mutate the objects in TS (not required in JS): playground

    const set = <T extends object>(obj: T): DeepSignal<T> => obj as DeepSignal<T>;
    
    store.b = { c: "3" }; // doesn't work in TS, but it works in JS
    store.b = set({ c: "3" }); // works fine in both

Of course, I'd prefer to avoid them 🙂

@melnikov-s
Copy link
Owner

melnikov-s commented Nov 28, 2022

Thanks for creating this issue!

It completely slipped my mind that TS will block you on assignment as it now believes that $whatever is part of the type and therefore should be present. I also did not deeply apply the type so only the top level properties had $ on them, oops!

I think you're right that there's no way to address this currently in TS. Either we go for an optional $ type which will require the consumer to assert that it is not null/undefined (!) or we are stuck with needing special setters to perform mutations.

I've opted to make the $ properties optional, I feel like that's the lesser of the two evils here especially since I figure the vast majority of the time consumers will use $ to pass signals to text nodes and dom attributes and react/preact will happy accept undefined there, so no need for a ! assertion.

I've published a new version (0.0.5) with these changes

@luisherranz
Copy link
Author

the vast majority of the time consumers will use $ to pass signals to text nodes and dom attributes and react/preact will happy accept undefined there

That makes sense.

What do you think, @DAreRodz?

@luisherranz
Copy link
Author

By the way, I updated the Stackblitz to the latest version and it works great now 🙂

@DAreRodz
Copy link

Hey @luisherranz, thanks for pinging me (although I'm pretty sure you and @melnikov-s have more context on this issue than me, haha 😅). Well, I can give my two cents.

If we have to choose either an optional $ for signals or a special setter to perform mutations, I will select the first option. For me, the main idea of a library like this would be to allow using an object ―based on signals― of any shape, as transparently as possible, as if it were a regular object. Having to use a setter wrapper in TS to modify the object, which is expected to happen frequently, makes me think such an API may be a bit cumbersome.

I also agree with @melnikov-s when he says that, most of the time, you pass signals to text nodes and dom attributes and that there are no issues with signals being typed as undefined. There could be problems, though, if you need them to be correctly typed (e.g., if you want to use peek()). I have to admit that I don't know how usual would be such cases, but I'm fine using ! for those. 🙂

@luisherranz
Copy link
Author

Great. Thank you, folks. I think we'll also go with that option.

@melnikov-s, I'm going to close this issue but if you find more TS problems with this solution in the future please let us know 🙂

@luisherranz
Copy link
Author

@melnikov-s, I've finally published the package: https://github.com/luisherranz/deepsignal/

I'm happy with how TypeScript ended up working. Thanks for your help.

I ended up using array.$[0] instead of array.$0 and peek(store, "prop") to peek, but other than that, it follows what we discussed here.

I haven't tested the library in a real project yet tho, so there may still be issues.

This is a Stackblitz example using TypeScript.

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