Feature: Add method handling to mixin processing in KDL #2100

Bashamega · 2025-08-03T08:10:54Z

related #2053

github-actions · 2025-08-03T08:11:06Z

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.

saschanaz · 2025-08-03T13:25:03Z

inputfiles/patches/dom.kdl

@@ -0,0 +1,12 @@
+// Manually moved from Document


cssom-view.kdl

saschanaz · 2025-08-03T13:29:30Z

src/build/patches.ts

+function handleMethod(child: Node): Partial<Method> {
+  const name = child.values[0] as string;
+  // Build the overrideSignatures array with the method signature string
+  const overrideSignatures = [


We should not use overrideSignatures unless it's explicitly used. Take a look at widlprocess.ts to see how to proceed: (this PR does not need to cover everything in that file)

TypeScript-DOM-lib-generator/src/build/widlprocess.ts

Lines 192 to 204 in 23819c7

} else if (member.type === "operation") {

const operation = convertOperation(member, result.exposed);

const { method } = result.methods;

if (!member.name) {

result.anonymousMethods!.method.push(operation);

} else if (method.hasOwnProperty(member.name)) {

method[member.name].signature.push(...operation.signature);

} else {

method[member.name] = operation as Browser.Method;

}

if (member.name) {

addComments(method[member.name], commentMap, i.name, member.name);

}

inputfiles/patches/cssom-view.kd

Co-authored-by: Kagami Sascha Rosylight <[email protected]>

…meters and update return types for elementFromPoint and elementsFromPoint methods. Enhance method handling in patches.ts to accommodate nullable return types and array signatures.

Bashamega · 2025-08-03T14:41:38Z

I've updated it, @saschanaz.
I used array instead of wrapping the return value with a sequence. If you still prefer your version, I can make it work with a regex. That said, I believe the current implementation is better for the parser.

saschanaz · 2025-08-03T15:07:47Z

inputfiles/patches/cssom-view.kd

@@ -0,0 +1,12 @@
+// Manually moved from Document


.kdl, not .kd 😛

Also maybe 2-space indent to be consistent with the rest of the code 👀

I have fixed the name

but not the indent 👀

I forgot, I have updated them

saschanaz · 2025-08-03T15:22:44Z

inputfiles/patches/cssom-view.kd

+        param x type=long
+        param y type=long
+    }
+    method elementsFromPoint returns="Element" array {


Would be nice to follow the syntax of

TypeScript-DOM-lib-generator/src/build/types.d.ts

Lines 1 to 8 in 23819c7

export interface Typed {

type: string | Typed[];

subtype?: Typed | Typed[];

nullable?: boolean;

overrideType?: string;

additionalTypes?: string[];

allowShared?: boolean;

}

But that doesn't seem possible as a property. Perhaps we should allow (but not force) type as a member for complex cases like this.

method elementsFromPoint { type sequence { type Element } param x type=long param y type=long }

Bashamega · 2025-08-04T12:12:38Z

I have updated it @saschanaz

Bashamega · 2025-08-05T09:46:23Z

Hello @saschanaz
Do you have any more requests for this PR?

saschanaz · 2025-08-06T07:57:12Z

inputfiles/patches/cssom-view.kdl

+// Manually moved from Document
+// See https://github.com/w3c/csswg-drafts/issues/5886 and https://github.com/w3c/csswg-drafts/issues/556
+interface-mixin DocumentOrShadowRoot {
+  method elementFromPoint returns=Element nullable=#true {


nullable is logically a property of the return type rather than the method, so in this case it would be also nicer to use separate type node as below.

saschanaz · 2025-08-06T07:59:53Z

src/build/patches.ts

+
+  const signature: Method["signature"] = [
+    {
+      type: type.name == "type" ? (type.values[0] as string) : returnType,


We'll probably get similar situation on other things than methods, this should probably be done in a helper function so that others can reuse it, something like handleTyped.

I have made it a helper function

saschanaz · 2025-08-06T08:01:46Z

src/build/patches.ts

+  const nullable = child.properties.nullable as boolean;
+
+  const params = child.children
+    .filter((c) => c.values[0] != "sequence")


It should filter in based on the type param, not filter out based on name?

Bashamega · 2025-08-07T06:23:01Z

I have updated it @saschanaz

Bashamega · 2025-08-10T07:37:26Z

Hello @saschanaz
Do you have any more requests for this PR?

Bashamega · 2025-08-26T05:23:19Z

Welcome back from vacation, @saschanaz!
I hope you had a great time. I’m excited to get this PR merged whenever you’re ready.

saschanaz

Sorry for the long delay!

saschanaz · 2025-08-26T20:30:20Z

src/build/patches.ts

+ * @param child The child node to handle.
+ */
+function handleMethod(child: Node): Partial<Method> {
+  const name = child.values[0] as string;


Too many as string, would be nice to have something like

function string(arg: unknown): string { if (typeof arg !== "string") { throw new Error(`Expected a string but found ${typeof arg}`); } return arg; }

saschanaz · 2025-08-26T20:32:11Z

src/build/patches.ts

+ */
+function handleMethod(child: Node): Partial<Method> {
+  const name = child.values[0] as string;
+  const type = child.children[0];


We should iterate and check types than assuming a position or using filter(), otherwise we can't throw on unexpected child. Just like line 120.

saschanaz · 2025-08-26T20:34:59Z

src/build/patches.ts

      }
+      case "method": {
+        const methodName = child.values[0] as string;
+        method[methodName] = handleMethod(child);


(Fine for now but eventually this will need to change to support overloads)

saschanaz · 2025-08-26T20:36:31Z

src/build/patches.ts

+  const name = isTyped ? (type.values[0] as string) : returnType;
+  const subType =
+    type.children.length > 0
+      ? { type: type.children[0].values[0] as string }


This should be a recursive call to handleTyped

Bashamega · 2025-08-27T05:17:44Z

No problem :)
I will take a look at the review today

Bashamega · 2025-08-27T08:53:33Z

I have updated it @saschanaz 😃

Bashamega · 2025-08-30T13:04:16Z

Hello @saschanaz,
Is there anything else that is missing here?

saschanaz

Let's remove returns for now and then this should be ok. Thanks!

BTW I think we got a lot more conversion boilerplate than I expected... I'll think about how to simplify.

saschanaz · 2025-09-07T18:59:24Z

src/build/patches.ts

+ */
+function handleMethod(child: Node): Partial<Method> {
+  const name = string(child.values[0]);
+  const returnType = child.properties.returns;


This is not used here, maybe add back when it's actually used but not now

saschanaz · 2025-09-07T19:01:39Z

src/build/patches.ts

+    switch (c.name) {
+      case "type":
+        if (typeNode) {
+          throw new Error(`Method "${name}" has multiple type nodes (invalid)`);


I actually want to allow this for union types, but for now this is ok as we don't have union types right now

saschanaz · 2025-09-07T19:20:04Z

src/build/patches.ts

+      ...handleTyped(typeNode, returnType),
+    },
+  ];
+  return { name, signature };


(This is fine for now but eventually we'll hit a problem when dealing with overloads)

Bashamega · 2025-09-08T04:30:59Z

Thank you for your review @saschanaz !
I have removed returns

Bashamega · 2025-09-17T14:31:14Z

Hello @saschanaz
Is there anything left here?

saschanaz · 2025-09-20T15:56:12Z

src/build/patches.ts

+  return arg;
+}
+
+function handleTyped(type: Node, returnType?: Value): Typed {


Still have unused parameter

We still use it for the subtype:
https://github.com/Bashamega/TypeScript-DOM-lib-generator/blob/0693d5d3d913903e66e808162f3dcee397681e9c/src/build/patches.ts#L50

Is it needed though? subtype would also be a type, so isTyped should be true, and then the second parameter won't be used, right?

I will update it

Bashamega · 2025-09-20T17:05:06Z

Done @saschanaz

saschanaz · 2025-09-20T18:58:08Z

LGTM

github-actions · 2025-09-20T18:58:21Z

There was an issue merging, maybe try again saschanaz. Details

saschanaz · 2025-09-20T19:17:48Z

LGTM (I feel like we are going to hit an anniversary of double LGTMing 😂)

github-actions · 2025-09-20T19:18:01Z

Sorry @saschanaz, this PR isn't open.

jakebailey · 2025-09-20T19:19:07Z

Well, I'm at least merging them

Feature: Add method handling to mixin processing in KDL

a62a343

saschanaz reviewed Aug 3, 2025

View reviewed changes

Bashamega added 3 commits August 3, 2025 16:39

-

e3f0a20

-

d7e96fb

-

a88fbed

saschanaz reviewed Aug 3, 2025

View reviewed changes

inputfiles/patches/cssom-view.kd Outdated Show resolved Hide resolved

inputfiles/patches/cssom-view.kd Outdated Show resolved Hide resolved

Bashamega and others added 3 commits August 3, 2025 16:50

Update inputfiles/patches/cssom-view.kd

2fcb5b9

Co-authored-by: Kagami Sascha Rosylight <[email protected]>

Update inputfiles/patches/cssom-view.kd

e6da694

Co-authored-by: Kagami Sascha Rosylight <[email protected]>

Refactor method signatures in cssom-view.kd to use long type for para…

37825cf

…meters and update return types for elementFromPoint and elementsFromPoint methods. Enhance method handling in patches.ts to accommodate nullable return types and array signatures.

saschanaz reviewed Aug 3, 2025

View reviewed changes

Bashamega and others added 5 commits August 3, 2025 18:31

Update and rename cssom-view.kd to cssom-view.kdl

c095db7

-

9682083

Merge Branches

6aa05dd

Merge branch 'main' into kdl/method

8f7ca28

-

0741374

saschanaz reviewed Aug 6, 2025

View reviewed changes

Bashamega and others added 4 commits August 7, 2025 08:58

Merge branch 'main' into kdl/method

7338aab

chore: split function

333fb26

filter on param

ea16eb1

-

fcd4188

Bashamega requested a review from saschanaz August 7, 2025 06:23

saschanaz suggested changes Aug 26, 2025

View reviewed changes

Bashamega and others added 4 commits August 27, 2025 08:54

Merge branch 'microsoft:main' into kdl/method

c5dcd9e

Refactor type handling in patches.ts to enforce string validation

58e6d23

recursively

8c2aa44

-

03d4afe

Bashamega requested a review from saschanaz August 27, 2025 08:53

saschanaz reviewed Sep 7, 2025

View reviewed changes

Bashamega and others added 2 commits September 8, 2025 07:18

Merge branch 'microsoft:main' into kdl/method

2cfa765

Remove returns

bd68c2d

Merge branch 'microsoft:main' into kdl/method

0693d5d

saschanaz reviewed Sep 20, 2025

View reviewed changes

Bashamega and others added 3 commits September 20, 2025 19:53

Update patches.ts

12566ec

fix

7da25a0

Lint

c4ba8c8

jakebailey approved these changes Sep 20, 2025

View reviewed changes

jakebailey merged commit 522dbd5 into microsoft:main Sep 20, 2025
5 checks passed

	} else if (member.type === "operation") {
	const operation = convertOperation(member, result.exposed);
	const { method } = result.methods;
	if (!member.name) {
	result.anonymousMethods!.method.push(operation);
	} else if (method.hasOwnProperty(member.name)) {
	method[member.name].signature.push(...operation.signature);
	} else {
	method[member.name] = operation as Browser.Method;
	}
	if (member.name) {
	addComments(method[member.name], commentMap, i.name, member.name);
	}

	export interface Typed {
	type: string \| Typed[];
	subtype?: Typed \| Typed[];
	nullable?: boolean;
	overrideType?: string;
	additionalTypes?: string[];
	allowShared?: boolean;
	}

Feature: Add method handling to mixin processing in KDL #2100

Feature: Add method handling to mixin processing in KDL #2100

Uh oh!

Conversation

Bashamega commented Aug 3, 2025

Uh oh!

github-actions bot commented Aug 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bashamega commented Aug 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Aug 4, 2025

Uh oh!

Bashamega commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Aug 7, 2025

Uh oh!

Bashamega commented Aug 10, 2025

Uh oh!

Bashamega commented Aug 26, 2025

Uh oh!

saschanaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bashamega commented Aug 27, 2025

Uh oh!

Bashamega commented Aug 30, 2025

Uh oh!

saschanaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bashamega commented Sep 8, 2025

Uh oh!

Bashamega commented Sep 17, 2025

Bashamega commented Aug 27, 2025 •

edited

Loading