Enhance handleProperty function to dynamically handle additional properties #2098
Conversation
…erties from child nodes
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
src/build/patches.ts
Outdated
| props.forEach((prop) => { | ||
| const value = child.properties[prop]; | ||
| if (value !== undefined) { | ||
| result[prop] = value as any; |
There was a problem hiding this comment.
Would be nice to have some type check, any should not be used.
Perhaps could use some helper:
import { parse, type Node, type Value } from "kdljs";
function optionalMember<const T>(prop: string, type: T, value?: Value) {
if (value === undefined) {
return {};
}
if (typeof value !== type) {
throw new Error(`Expected type ${value} for ${prop}`);
}
return { [prop]: value as T extends "string" ? string : T extends "number" ? number : T extends "boolean" ? boolean : never };
}
const result = {
name: child.values[0] as string,
...optionalMember("exposed", "string", child.properties?.exposed),
...optionalMember("optional", "boolean", child.properties?.optional),
...optionalMember("overrideType", "string", child.properties?.overrideType),
};There was a problem hiding this comment.
I have updated it for property and mixins
inputfiles/patches/eventhandlers.kdl
Outdated
| property ontouchcancel optional=#true | ||
| property ontouchend optional=#true | ||
| property ontouchmove optional=#true | ||
| property ontouchstart optional=#true |
There was a problem hiding this comment.
Some context comment would be nice for touch events, which would be nicer as touch-events.kdl:
Touch event handlers are intentionally hidden in non-mobile web browsers.
See https://w3c.github.io/touch-events/#dfn-expose-legacy-touch-event-apis.
inputfiles/patches/eventhandlers.kdl
Outdated
| property ontouchend optional=#true | ||
| property ontouchmove optional=#true | ||
| property ontouchstart optional=#true | ||
| property onerror overrideType=OnErrorEventHandler |
There was a problem hiding this comment.
This can go events.kdl, as this should ultimately also be autogenerated rather than overridden.
inputfiles/patches/events.kdl
Outdated
| // Touch event handlers are intentionally hidden in non-mobile web browsers. | ||
| // See w3c.github.io/touch-events#dfn-expose-legacy-touch-event-apis. | ||
| property ontouchcancel optional=#true | ||
| property ontouchend optional=#true | ||
| property ontouchmove optional=#true | ||
| property ontouchstart optional=#true |
There was a problem hiding this comment.
These should go to touch-events.kdl. With that LGTM.
There was a problem hiding this comment.
I have moved them to a separate file
|
Is this LGTM? @saschanaz |
|
Sorry @Bashamega, you don't have access to these files: |
inputfiles/patches/touch-events.kdl
Outdated
| property ontouchmove optional=#true | ||
| property ontouchstart optional=#true | ||
| property onerror overrideType=OnErrorEventHandler | ||
| } |
There was a problem hiding this comment.
missing final newline and 2-space indent
inputfiles/patches/touch-events.kdl
Outdated
| property ontouchend optional=#true | ||
| property ontouchmove optional=#true | ||
| property ontouchstart optional=#true | ||
| property onerror overrideType=OnErrorEventHandler |
There was a problem hiding this comment.
And onerror should go back to events.kdl
|
I have moved it, and formatted all KDL files @saschanaz |
inputfiles/patches/cssom-view.kdl
Outdated
| param x type=long | ||
| param y type=long |
There was a problem hiding this comment.
Wait, why are two PRs merged here? 🤔
There was a problem hiding this comment.
This should be removed. Which I did
…peScript-DOM-lib-generator into optional-properties
This reverts commit 501d38e.
|
I have fixed it @saschanaz |
|
LGTM |
|
There was an issue merging, maybe try again saschanaz. Details |
|
LGTM |
|
Merging because @saschanaz is a code-owner of all the changes - thanks! |
related #2053