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.

Sign up for GitHub

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+348 −55
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
488bfff
feat(engine): support imperative wire adapters & fix wire adapter con…
aheber Dec 27, 2024
0a0e675
Accept recommended changes `ReactivePropertyOfComponent`
aheber Jan 7, 2025
ac422be
Accept changes in string pattern for prefixing
aheber Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/@lwc/engine-core/src/framework/decorators/wire.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import { updateComponentValue } from '../update-component-value';
import type { LightningElement } from '../base-lightning-element';
import type {
ConfigValue,
ConfigWithReactiveValues,
ContextValue,
ReplaceReactiveValues,
WireAdapterConstructor,
} from '../wiring';

Expand Down Expand Up @@ -59,15 +59,17 @@ interface WireDecorator<Value, Class> {
* }
*/
export default function wire<
ReactiveConfig extends ConfigValue = ConfigValue,
ExpectedConfig extends ConfigValue = ConfigValue,
Value = any,
Context extends ContextValue = ContextValue,
Class = LightningElement,
>(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
adapter: WireAdapterConstructor<ReplaceReactiveValues<ReactiveConfig, Class>, Value, Context>,
adapter:
| WireAdapterConstructor<ExpectedConfig, Value, Context>
| { adapter: WireAdapterConstructor<ExpectedConfig, Value, Context> },
// eslint-disable-next-line @typescript-eslint/no-unused-vars
config?: ReactiveConfig
config?: ConfigWithReactiveValues<ExpectedConfig, Class>
): WireDecorator<Value, Class> {
if (process.env.NODE_ENV !== 'production') {
assert.fail('@wire(adapter, config?) may only be used as a decorator.');
Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/engine-core/src/framework/wiring/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ export { createContextProviderWithRegister, createContextWatcher } from './conte
export {
ConfigCallback,
ConfigValue,
ConfigWithReactiveValues,
ContextConsumer,
ContextProvider,
ContextProviderOptions,
ContextValue,
DataCallback,
ReplaceReactiveValues,
WireAdapter,
WireAdapterConstructor,
WireAdapterSchemaValue,
Expand Down
61 changes: 29 additions & 32 deletions packages/@lwc/engine-core/src/framework/wiring/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,38 +88,35 @@ export type RegisterContextProviderFn = (
onContextSubscription: WireContextSubscriptionCallback
) => void;

/** Resolves a property chain to the corresponding value on the target type. */
type ResolveReactiveValue<
/** The object to search for properties; initially the component. */
Target,
/** A string representing a chain of of property keys, e.g. "data.user.name". */
Keys extends string,
> = Keys extends `${infer FirstKey}.${infer Rest}`
? // If the string is "a.b.c", check if "a" is a prop on the target object
FirstKey extends keyof Target
? // If "a" exists on the target, check `target["a"]` for "b.c"
ResolveReactiveValue<Target[FirstKey], Rest>
: undefined
: // The string has no ".", use the full string as the key (e.g. we've reached "c" in "a.b.c")
Keys extends keyof Target
? Target[Keys]
: undefined;

/**
* Detects if the `Value` type is a property chain starting with "$". If so, it resolves the
* properties to the corresponding value on the target type.
* build a union of reactive property strings that would be compatible with the expected type
*
* exclude properties that are present on LightningElement, should help performance and avoid circular references that are inherited
*
* when looking for compatible types mark all optional properties as required so we can correctly evaluate their resolved types
*/
type ResolveValueIfReactive<Value, Target> = Value extends string
? string extends Value // `Value` is type `string`
? // Workaround for not being able to enforce `as const` assertions -- we don't know if this
// is a true string value (e.g. `@wire(adapter, {val: 'str'})`) or if it's a reactive prop
// (e.g. `@wire(adapter, {val: '$number'})`), so we have to go broad to avoid type errors.
any
: Value extends `$${infer Keys}` // String literal starting with "$", e.g. `$prop`
? ResolveReactiveValue<Target, Keys>
: Value // String literal *not* starting with "$", e.g. `"hello world"`
: Value; // non-string type

export type ReplaceReactiveValues<Config extends ConfigValue, Component> = {
[K in keyof Config]: ResolveValueIfReactive<Config[K], Component>;
type ReactivePropertyOfComponent<Target, ExpectedType> = PrefixDollarSign<
PropertiesOfType<Required<Omit<Target, keyof LightningElement>>, ExpectedType>
>;

/** utility type */
type PrefixDollarSign<T extends string> = `$${T}`;

/** recursively find all properties on the target that are of a compatible type, returning their paths as strings */
type PropertiesOfType<Target, ExpectedType> = {
[K in keyof Target]: Target[K] extends ExpectedType
? `${Extract<K, string>}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: Target[K] extends object // If the value is an object, recursively check its properties
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? PropertiesOfType<Target[K], ExpectedType> extends infer R // Check if any property in the object matches `U`
? R extends never
? never // If no compatible keys are found, exclude the key
: `${Extract<K, string>}.${Extract<R, string>}` // If compatible nested keys are found, include the key
: never
: never;
}[keyof Target];

/** wire decorator's config can be the literal type defined or a property from component that is compatible with the expected type */
export type ConfigWithReactiveValues<Config extends ConfigValue, Comp> = {
// allow the original config value and also any valid reactive strings
[K in keyof Config]: Config[K] | ReactivePropertyOfCompoent<Comp, Config[K]>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 $.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
Loading