#How can I infer the type of this function more precisely?

1 messages · Page 1 of 1 (latest)

mighty cedarBOT
#
german.jablo#0
Playground Link

Preview:```ts
type State = {
[Key in string]?: string | number | boolean | State
}

class MyNode {
getState<T extends State>(
key: Extract<keyof T, string>
): T[typeof key] | undefined {
throw new Error("not implemented yet")
}

setState<T extends State>(
key: Extract<keyof T, string>,
value: T[typeof key] | undefined
) {}
...```

You can choose specific lines to embed by selecting them before copying the link.
summer mulch
#

I would like the value to be inferred based on the generic and the argument.
As a clarification, I would like a solution that does not involve passing 2 arguments: getState<MyState, "myPluginColor">`

slim scarab
#

You have to use a generic type parameter for key.

#

It could be in a different part of the call, but it has to exist.

#

(As a side note, getState is impossible to implement safely other than throwing or returning undefined, because T can be literally anything)

summer mulch
#

Oh, I pasted an old playground. It's still the same, just change the key argument:

mighty cedarBOT
#
german.jablo#0
Playground Link

Preview:```ts
type State = {
[Key in string]?: string | number | boolean | State
}

class MyNode {
getState<T extends State>(
key: keyof T
): T[typeof key] | undefined {
throw new Error("not implemented yet")
}

setState<T extends State>(
key: keyof T,
value: T[typeof key] | undefined
) {}
...```

You can choose specific lines to embed by selecting them before copying the link.
summer mulch
#

You have to use a generic type parameter for key.

Really? ugh, what a bummer

eternal ridge
#

i suspect you want something like this:

mighty cedarBOT
#
mkantor#0
Playground Link

Preview:```ts
type State = {
[Key in string]?: string | number | boolean | State
}

class MyNode<T extends State> {
state
constructor(state: Partial<T> = {}) {
this.state = state
}

getState<K extends keyof T>(
key: K
): T[K] | undefined {
return this.state[key]
...```

You can choose specific lines to embed by selecting them before copying the link.
summer mulch
#

(As a side note, getState is impossible to implement safely other than throwing or returning undefined, because T can be literally anything)
WDYM with that?

eternal ridge
slim scarab
#

If you look at the compiled JS:

const state1 = node.getState<MyState>("myPluginColor")
const state2 = node.getState<MyOtherState>("myPluginColor")

It will compile to:

const state1 = node.getState("myPluginColor")
const state2 = node.getState("myPluginColor")

And let's say MyState.myPluginColor is a string but MyOhterState.myPluginColor is a number, how is it possible for node.getState("myPluginColor") to return different values?

#

node.getState has no way to know what T is, it's erased from compiled JS.

summer mulch
# eternal ridge i suspect you want something like this:

Yes... I thought that, but the class is meant to be extended by third parties and to extend state as they wish. Module augmentation could be used but the Meta team didn't like that idea very much.
For context, the MyNode class is LexicalNode... this is for the Lexical text editor.

summer mulch
eternal ridge
#

how could this possibly work? other than the problem with getState, everyone is free to clobber anyone else's state because they can pass anything for T in setState

slim scarab
#

Well it's unsafe, because state1 and state2 are different types which they clearly cannot be.

summer mulch
#
  1. It's not something that you expect a lot of independent teams to use together. Only for things like CMS.
  2. It's advised to prefix keys with the plugin name to avoid collisions
#

I intend the generic to be more of an assertion helper.

slim scarab
#

This is a design issue that T can arbitrarily change as caller wish but that's not possible for MyNode to take that into account, and that's why the whole state1 and state2 problem occurs.
One way to fix the design issue is to disallow T from changing, and @eternal ridge's solution of lifting T up to MyNode<T> solves it (which also solves your initial question of how to make the return value depend on key).

summer mulch
#

I understand what you're saying. It's a tradeoff to avoid having to use inheritance, which brings a lot of problems currently in Lexical.

eternal ridge
#

can you go into more detail as to what this looks like?

third parties and to extend state as they wish

#

are they subclassing Node, getting an already-created node instance, something else?

summer mulch
#

Yes, for example users could insert arbitrary properties to the subclasses that come with the editor (TextNode, ParagraphNode, etc., which all extend LExicalNode)
This shouldn't be the most common case, since the editor provides the properties that people will want to use 90% of the time, but when they don't, extending nodes becomes a pain and isn't composable.

eternal ridge
#

if they are creating their own subclasses, why do they need these type parameters? they can use whatever internal state they wish in the subclass as long as it doesn't directly conflict with anything in the superclass

summer mulch
#

Subclasses are provided by the library as well. You can subclass them again, but:

  1. Everything you have to do to make this work is incredibly verbose.
    See the collapsible section of this comment for context:
    https://github.com/facebook/lexical/pull/6929#issuecomment-2545988630
  2. It's not composable. If 2 people extend the same node, there will be errors at runtime.
    For context, see the PR linked above
eternal ridge
#

thanks for the context, i'll take a look

summer mulch
eternal ridge
#

i don't think i'm connecting all the dots from that PR comment/the verboseness problem up to the proposed API. how does the "state" fit into all of this?

maybe another angle will work better: you stubbed out the implementations of getState and setState in the playground. can you show how they might actually be implemented? even just pseudocode would help

slim scarab
#

I imagine the getState/setState would just store into a Map<string, unknown>. The return type for getState/setState is purely for convenience and unsafe.

#

Kind of like how some libraries do axios.get<T>().

eternal ridge
#

oh, like they're saying every single node would come with a bag of arbitrary values? but why? if the third parties are writing their own subclasses anyway, they can just define their own nicely-typed state, no?

#

is there some reason the state needs to be owned by library rather than the end user?

mighty cedarBOT
#
german.jablo#0
Playground Link

Preview:```ts
type State = {
[Key in string]?: string | number | boolean | State
}

class LexicalNode {
__state: State = {}

getState<T extends State = State>(
key: keyof T
): T[keyof T] | undefined {
return (this.__state as T)[key]
}

setState<T extends State = State>(
key: keyof T,
value: T[keyof T]
) {
...```

You can choose specific lines to embed by selecting them before copying the link.
eternal ridge
# mighty cedar

thanks, can you also show me what it looks like when third parties extend LexicalNode?

summer mulch
#

The point is that users don't have to subclass.

eternal ridge
#

oh

eternal ridge
#

how does this solve the composition/conflict problem though?

summer mulch
#

Sorry. I meant that nowadays you can subclass and people do it, but it brings more problems than an unknown state would bring.
Regarding composability, from the PR: "extending nodes is not composable. For example, in Payload the community can create their own plugins. If two of them extend the same node, it doesn't work"

eternal ridge
#

i'm not sure what specific problem that refers to, but either way you still have issues:

mighty cedarBOT
#
mkantor#0
Playground Link

Preview:```ts
...
// let's say your plugin and my plugin both have access to this node:
const node = new LexicalNode();

// my plugin:
node.setState<{ id: string }>('id', 'abc')

// your plugin:
node.setState<{ id: number }>('id', 123)

// later in my plugin:
const id = node.getState<{ id: string }>('id')
// ^?
// but id is actually a number at runtime, because your plugin overwrote it```

eternal ridge
#

better would be to give each plugin its own "namespace"

summer mulch
#

yes, I know. That's why I said that the recommendation is going to be to prefix the keys with the plugin name. Example: myPlugin-bgColor: "red"

#

It's a tradeoff

eternal ridge
#

i guess i don't know Lexical so would have trouble understanding what it's trading off against exactly

#

but that's okay

eternal ridge
#

being able to see as MyThing in the source code tells me exactly where to look when something goes wrong

slim scarab
#

Agree, we also have the following for this reason.

#

!:that

#

!*:that

#

🤦

eternal ridge
#

!:that*

mighty cedarBOT
#
tjjfvi#0
`!tjjfvi:thats-no-generic-its-a-type-cast`:

When you use a type parameter in the function, it should generally be inferrable from the arguments.

For example, this is a misuse of generics:

function getMeA<T>(): T {
   /* magic */
}

because getMeA<string>() and getMeA<number>() compile to the same code at runtime, there's so way to implement this function safely (other than always throwing); this is just a type cast in disguise. Instead of using a generic here, you should return unknown, and cast at the call site if necessary, to be clear it's an unsafe operation:

-function getMeA<T>(): T {
+function getMeA(): unknown {
    /* magic */
 }

-getMeA<number>()
+getMeA() as number

One exception to this rule is if you're returning a possibly-empty container of T. For example, these are all perfectly safe, even though the generic can't be inferred from the parameters:

function emptyArray<T>(): Array<T> {
  return []
}

function useRef<T>(): { current?: T } {
  return {}
}
slim scarab
#

There we go.

summer mulch
#

I get the point, but it's more error-prone at times.

mighty cedarBOT
#
german.jablo#0
Playground Link

Preview:```ts
type State = Record<string, string | number | boolean>

class LexicalNode {
__state: State = {}

getState<T extends State = State>(
key: keyof T
): T[keyof T] | undefined {
throw new Error("not implemented yet")
}

setState<T extends State = State>(
key: keyof T,
value: T[keyof T] | undefined
) {}
...```

You can choose specific lines to embed by selecting them before copying the link.
summer mulch
#

Or even something simpler, which can return "something" or undefined, I prefer to do get<"something">(...) than get(something) as "something" | undefined.
Some people might forget to handle the undefined case there.
But I understand that it's a matter of taste.

slim scarab
#

I'm kind of tuned out of the conversation half way, what are the design goals and constraints? From skimming, do you just need:

  • You want third party code to be able to extend a node and add states that are accessible via getState/setState.
  • You don't want the mechanism for extending to be inheritance.
summer mulch
#

Yes I guess that's it

slim scarab
#

Then I would suggest declaration merging instead:

mighty cedarBOT
#
nonspicyburrito#0
Playground Link

Preview:```ts
interface LexicalNodeState {}

class LexicalNode {
__state: Record<string, string | number | boolean> =
{}

getState<K extends keyof LexicalNodeState>(
key: K
): LexicalNodeState[K] | undefined {
throw new Error("not implemented yet")
}

setState<K extends keyof LexicalNodeState>(
key: K,
value: LexicalNodeState[K] | undefined
) {}
...```

You can choose specific lines to embed by selecting them before copying the link.
slim scarab
#

Third party code that wants to add additional states can just merge into LexicalNodeState and get full type safety.

#

Actually, you don't even need getState/setState if you don't need to do anything else in those methods other than changing the state.

summer mulch
slim scarab
slim scarab
mighty cedarBOT
#
nonspicyburrito#0
Playground Link

Preview:```ts
interface LexicalNodeState {}

class LexicalNode {
state: Partial<LexicalNodeState> = {}
}

// Consumer

interface LexicalNodeState {
foo: string
bar: number
}

const node = new LexicalNode()
const foo = node.state.foo
const bar = node.state.bar
node.state.doesNotExist
...```

You can choose specific lines to embed by selecting them before copying the link.
summer mulch
#

Wait, interfaces are global and automatically combined???

eternal ridge
slim scarab
#

They are not global, but multiple declaration of the same interface will get merged, so the third party code could do:

declare module 'the-lexical-package' {
    interface LexicalNodeState {
        foo: string
        bar: number
    }
}

And that would merge into it.

#

(It's a potentially dangerous feature though, so abuse use at your own risk)

summer mulch
#

Wow, TIL.

It looks great, although it makes me a little suspicious.
What risks or dangers can you foresee?
How can I make sure that tsc is "catching" all the other interface definitions if they are not global?
Is it always necessary to declare module?

#

Something I am considering though is that it would be possible for a user to want to use one state for TextNode, another for ParagraphNode, etc., which would imply that in the Lexical source code you would have to define interfaces for each class instead of just for the base class LexicalNode...

slim scarab
#

The dangerous point is pretty simple, you see all the libraries out there using interfaces and not knowing consumer can just merge into them and break everything.

#

Not sure about what you mean by tsc catching or the last point.

eternal ridge
#

i think it's basically the same conflicting state problem we talked about before

summer mulch
#

I think the difference is that you notice collisions at compile time.

eternal ridge
#

only if they occur in the same package, i think? if pluginA and pluginB both happen to use the same state key and my app depends on both of those plugins i am in for a bad time, and i don't think the compiler would complain about it

#

i guess maybe if i disable skipLibCheck i might see an error? not totally certain actually

summer mulch
#

hm, yes probably

eternal ridge
#

this may be part of the tradeoff balance sheet, but is there any way you can have users register their plugins? you could have an API like registerPlugin('myPlugin', …) and then handle the prefixing/namespacing internally so it can't be violated (and throw an error on colliding plugin names)