-
Notifications
You must be signed in to change notification settings - Fork 394
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
fix(types) enable type inference for config property #2802
Conversation
Adding in the ability to infer config property structures from the WireAdapter definition. This enabled type checking on the allowed properties of the passed WireAdapter.
Thanks for opening the PR!
This would be helpful, thanks. Do you mean it helps with VS code TypeScript checking or something along those lines? |
config?: WireConfigValue | ||
export function wire<T, R>( | ||
adapter: WireAdapterConstructor<T> | LegacyWireAdapterConstructor<T, R>, | ||
config?: T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My TypeScript-Fu is weak, but if I'm not mistaken, isn't this broadening rather than narrowing the allowed type here? Should it be something like config?: T extends WireConfigValue
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding another comment of where I think that should be applied. (WireAdapter definition)
interface WireAdapter { | ||
update(config: WireConfigValue, context?: ContextValue): void; | ||
interface WireAdapter<T> { | ||
update(config: T, context?: ContextValue): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nolanlawson, I think this one should be config?: T extends WireConfigValue
(here on the WireAdapter)(I thought I had that already but this I missed it in my copying from a few different sources). It would only make sense to apply it on the actual wire
's config definition if it also applied to the LegacyWireAdapterConstructor
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aheber Looking back through this PR (and I apologize for the late review), one concern I have is that these types are not consistent with the types defined in @lwc/engine
itself, e.g.:
lwc/packages/@lwc/engine-core/src/framework/wiring/types.ts
Lines 22 to 26 in d004e93
export interface WireAdapterConstructor { | |
new (callback: DataCallback): WireAdapter; | |
configSchema?: Record<string, WireAdapterSchemaValue>; | |
contextSchema?: Record<string, WireAdapterSchemaValue>; | |
} |
Long-term, I think our goal is to make the lwc
package merely a wrapper that exports other packages (like @lwc/engine
). So rather than have the canonical types defined in lwc
, it seems better to have them defined at the source (@lwc/engine
).
In the short term, I think it would be acceptable to just update the types in both places, though. I took a crack at it, but ran into some typing issues in @lwc/engine
. (I'm not much of a TypeScript wizard. 😅)
I was just investigating the derived types using |
Closing this PR as it's quite outdated and its goals have been otherwise addressed. |
Adding in the ability to infer config property structures from the WireAdapter definition. This enabled type checking on the allowed properties of the passed WireAdapter. I'm also including a convenience type for applying correct typing to the return value from the wire adapter.
Details
Happy to show a demo or screenshots of the value this provides. Extracting the config structure from the WireAdapter that is provided as the first param and using it to type the second param. Should work with both WireAdapterConstructor and LegacyWireAdapterConstructor, have functional tests on my machine that I can provide.
It is also possible that these changed need to be applied inside of the engine also, not just in the delivered typing.
The major point of this change is to inform the
wire
decorator of the "shape" of theconfig
param, in this example it informs thatrecordId
is the only allowed property. Passing any different properties, or not passing that property should cause an error condition on type-enforced code.This leaves a gap, there isn't currently a way to have the decorator apply typing to the decorated property (method or value). If Decorators ever get solidified in Javascript then I expect Typescript will provide a way to accomplish this and can revisit then.
To help with that scenario of typing the returned value I also provide a convenience type that can be used to apply typing to wired properties or methods to extract and deliver the typed return value and apply it automatically to the "data" property of the output. There is a chance that this is an inappropriate place for that change and it should be applied at a higher level, Feedback welcome. (I also cause the error property to be of the type "FetchResponse" but that might be a Salesforce specific concept and not an LWC concept).
Applied as:
Does this pull request introduce a breaking change?
Zero change to how the engine/system works. Only in the exposed type interfaces.
Biggest risk I can see is if anyone is using type enforcement and not passing valid config types, this will flag it. Could expose what have likely been errors but are not properly validated.
There are a lot of gaps in my knowledge here, I might have some specific expectations that aren't met by all use cases. Will need some additional review and validation.
Does this pull request introduce an observable change?