feat(engine): support imperative wire adapters & fix wire adapter config handling

[aheber]

Details

Improve the type definition of wire decorator to support Imperative functions that also carry wire adapters.

This change also improves the type checking for the config param. The types are more closely validated, error messages are scoped better and are more concise.

Does this pull request introduce a breaking change?

• 💔 Yes, it does introduce a breaking change.

This is a breaking change for TypeScript users only. This expands what is allowed to be passed to the decorator as an adapter and tightens config types.

Does this pull request introduce an observable change?

• 🔬 Yes, it does include an observable change.

wire decorator type checking is different. No runtime behavior changes.

[nolanlawson]

/nucleus test

[nolanlawson]

Breaking change, so marking as v9 and "no merge."

Thanks a bunch for this!

[nolanlawson]

Fixes #5082

[aheber]

Awesome! Look forward to it landing.

[wjhsf]

This is super cool! I especially like that this makes the relevant props pop up as string suggestions in the IDE. There are a few edge cases to consider, but I think overall this is a move in the right direction. (In addition to, well, the obvious feature that we were lacking...)

[wjhsf]

It's possible to do $array.0.prop (playground), so this will also need to support numbers, somehow.

[wjhsf]

If Config[K] is string, then an invalid reactive string (e.g. "$number") will pass validation, which is a regression from the current type def.

[aheber]

@wjhsf I did notice that. I didn't know what was valid here so I didn't block it. What does the framework do if I pass a string that is patterned like a reactive string but is really just intended to be a string?

If the framework rejects it then I can go back and add it in. Shouldn't be too hard to alter the type if it is string to ensure it doesn't start with a $.

[aheber]

I don't know that this is a regression, if you look at my example sheet here you can see that I've added some examples of invalid reactive prop references passing validation.

Typescript Playground for LWC Wire Adapter Typings

Such as: @wire(getBookWireAdapter, { id: "$propDoesNotExist", title: "$alsoNotHere" }) invalidConfigWithBinds?: Book where both the number and string props accepted anything that looks like a reactive prop.

I do think it should be pretty easy to filter out if you can confirm that LWC will choke on it.

[wjhsf]

So there's a subtle thing happening here, {title: "$alsoNotHere"} gets inferred as {title: string}. And a string type must always pass validation, because we can't definitively tell whether or not it's a valid reactive string. If you do {title: "$alsoNotHere"} as const, then you get the string literal types as expected. When using as const, you get an error with the current type def, but not with the proposal. That's the regression I'm concerned about. (Not too concerned, though. I'd probably still ship this PR even if we can't fix it.)

[aheber]

@wjhsf to confirm. LWC has a problem if a "reactive-like" string is provided but it doesn't map to a component property?
If that is the case then it is safe to restrict this and I will make that change.

[wjhsf]

If a prop doesn't exist, the framework will use undefined.

[aheber]

OK, that seems clear. A literal string that starts with a $ but doesn't match a valid prop is still treated as a reactive prop and resolves to undefined, not back to the literal string value. I agree it makes sense to exclude those if we can.

I've been trying to define a type that does that. I've tried several things, some of them look like they worked in the past but don't work anymore (or the articles I pulled them from were simply wrong).

I was playing with versions of

type IsNotReactive = string extends `$${string}` ? never : string;

but haven't been able to make anything stick. If you have some ideas of how to type this please let me know. I think the biggest part of the problem is the strategy used in this PR is deriving types from the adapter's stated Config, not the provided values. So the type resolution happens prior to the config values being considered. I would need some way of saying "I see you said allows string type, let me massage that to be any of these prop strings or any string as long as it doesn't start with $" but I haven't been able to get so far as defining a type that ensures "string that doesn't start with $"

Maybe we can find a middle ground that somehow has two layers of type checking, even though it wouldn't be the best error messages, layer 1 is extract valid types based on the config, layer 2 determines the valid strings based on the values passed to the config param, using some of the previous config. The generic type resolution strategy may be hard to manage from both directions.

This open issue (from 2022) is presumably the feature we're looking for to supports this, it doesn't seem to have a lot of traction right now.
microsoft/TypeScript#49867

I think this is the "official" issue for negated types which seems to match our goal
microsoft/TypeScript#4196

[wjhsf]

Yeah, I don't think we can validate "string but not invalid $ string" with this approach. We can mitigate the issue with something like this. It doesn't change the validation at all, but at least it shows the $ props in the IDE autocomplete suggestions.

export type ConfigAndReactiveValues<Config extends ConfigValue, Comp> = {
    // allow the original config value and also any valid reactive strings
    [K in keyof Config]:
        | (string extends Config[K] ? Config[K] & { __hack?: never } : Config[K])
        | ReactivePropertyOfCompoent<Comp, Config[K]>;
};

(Explanation: string | 'foo' gets collapsed to string, but (string & {}) | 'foo' doesn't because of the & {}. The & {} essentially means "non-nullable"; since every string is by definition not null, every string is still valid. But because the type doesn't collapse, 'foo' is still part of the union, so it gets suggested.)

[wjhsf]

In general, I like this approach better, despite this issue. But we want to avoid breaking existing usage, so I'll have to do some analysis to see what's likely to explode if we make this change.

[wjhsf]

If ExpectedType is object, then we'll never recursively search for props. Definitely an edge case, but maybe something like this might work?

[K in keyof Target]:
 | (Target[K] extends ExpectedType ? `${Extract<K, string>}` : never)
 | (Target[K] extends object ? /* recursion */ : never)

[wjhsf]

Poked around and came up with this:

type Values<T> = { [K in keyof T]: T[K] }[keyof T];

/** recursively find all properties on the target that are of a compatible type, returning their paths as strings */
type PropertiesOfType<Target, ExpectedType> = Values<{
    [K in Exclude<keyof Target, symbol>]:
        | (Target[K] extends ExpectedType ? `${K}` : never)
        // If the value is an object, recursively check its properties
        | (Target[K] extends object ? `${K}.${PropertiesOfType<Target[K], ExpectedType>}` : never);
}>;

playground

[aheber]

@wjhsf I think I mostly like that. I'm a bit worried about the type Obj = PropertiesOfType<Test, object> example because it is pulling functions as options. Does that feel like something we should specifically avoid? What will LWC do if we reference a function as a reactive prop?

[wjhsf]

You're right, it does feel weird to be pulling in functions. But I'm not too concerned, because it seems unlikely that anyone is using plain object as a type. They'd be more likely to use a more specific object, like {id: number}, which the methods won't match.

The framework doesn't do anything with the reactive values, it just passes them through to the wire adapter implementation. So if a wire adapter asks for an object, then I guess it's technically not wrong if someone provides it with a function. It's up to the adapter implementation to ask for the correct thing. But even if an adapter does ask for object, then the end user will probably have a $data prop or something that they'll know to use. The impact of the extra types is just unnecessary autocomplete suggestions, not really anything breaking.

[aheber]

I can get behind that, hopefully it never comes up.

[wjhsf]

🤔 One thing that I don't like about the current implementation is that it requires modern decorators, but most SFDX projects are still configured to use legacy/experimental decorators. I wonder if this approach means we could restore support for experimental decorators.

[wjhsf]

@aheber do you think you could split the .adapter change into a separate PR, using the current type def? Then we could add that as a feature (should be able to land in v8) and focus on the refactor separately (would land in v9).

[wjhsf]

// `Class` appears unused, but by doing `Name extends keyof Class`, the result type preserves
// the optional/required status of each prop. If we just do `Name extends PropertyKey`, that
// necessary information is lost.
type ExpectedTarget<Value, Class, Name extends keyof Class> = {
    [K in Name]: Value;
};

interface WireDecorator<Value, Class, Name extends keyof Class> {
    (
        target: unknown,
        context: // A wired prop doesn't have any data on creation, so we must allow `undefined`
        | ClassFieldDecoratorContext<Class, Value | undefined>
            | ClassMethodDecoratorContext<
                  Class,
                  // When a wire adapter is typed as `WireAdapterConstructor`, then this `Value`
                  // generic is inferred as the value used by the adapter for all decorator contexts
                  // (field/method/getter/setter). But when the adapter is typed as `any`, then
                  // decorated methods have `Value` inferred as the full method. (I'm not sure why.)
                  // This conditional checks `Value` so that we get the correct decorator context.
                  Value extends (value: any) => any ? Value : (this: Class, value: Value) => void
              >
            // The implementation of a wired getter/setter is ignored; they are treated identically
            // to wired props. Wired props don't have data on creation, so we must allow `undefined`
            | ClassGetterDecoratorContext<Class, Value | undefined>
            | ClassSetterDecoratorContext<Class, Value>
    ): void;
    (target: ExpectedTarget<Value, Class, Name>, prop: Name): void;
    (
        target: Class,
        prop: Name,
        descriptor: // getter/setter
        | TypedPropertyDescriptor<Value>
            // method with param
            | TypedPropertyDescriptor<(value: Value) => any>
            // method with no param
            | TypedPropertyDescriptor<() => any>
    ): void;
}

Here's an updated type for WireDecorator (along with a new generic param Name added to the wire function). It works for our current test cases when using experimental decorators, but the last overload makes it explode when using modern decorators. I think it's probably something to do with the way the signature changes the inferred generic params, but I don't have time to look into it now.

[wjhsf]

I wonder if this approach means we could restore support for experimental decorators.

Turns out we can't, because of #5087 (comment). The LWC compiler required decorators to be preserved, and tsc only preserves modern decorators, not experimental.

[aheber]

I've isolated the change for Imperative support into this PR: #5132

We can continue to refine this without holding up that compatibility fix.