Implement support for callback functions

[ExE-Boss]

This implements callback functions similarly to how callback interfaces were implemented in #172.

I would’ve preferred if this were included in WebIDL2JS v16.0.

Depends on:

• Fix ECMAScript to IDL Promise type conversion #219

---

Supersedes and closes #123

review?(@domenic, @TimothyGu, @Sebmaster)

[domenic]

Thanks for jumping on this!

[domenic]

Optional, but if it's easy, it would be nice to remove the unused callResult variable from this case (and construct).

[TimothyGu]

Be sure to test this on jsdom before making a release. What blocked my other patch was that existing jsdom code didn't like this change.

[ExE-Boss]

I’ll begin work on the JSDOM side of this once jsdom/jsdom#2884 is merged.

[TimothyGu]

Suggested change

	CallbackFunction.prototype.type = "callback";
	CallbackFunction.prototype.type = "callback function";

Change others as appropriate.

[ExE-Boss]

This is to match WebIDL2’s type property: https://github.com/w3c/webidl2.js/blob/561efd0007038cc94ec2da8fcfa11623fe94101a/lib/productions/callback.js#L22-L24.

[domenic]

I stress-tested this against whatwg/streams#1035 and found some bugs with the promise conversion code.

[domenic]

This is incorrect.

[ExE-Boss]

This code is generated by:

webidl2js/lib/types.js

Lines 330 to 349 in 7d45dfc

	function generatePromise() {
	let handler;
	if (idlType.idlType[0].idlType === "void") {
	// Do nothing.
	handler = "";
	} else {
	const conv = generateTypeConversion(ctx, "value", idlType.idlType[0], [], parentName,
	`${errPrefix} + " promise value"`);
	requires.merge(conv.requires);
	handler = `
	${conv.body}
	return value;
	`;
	}
	str += `
	${name} = Promise.resolve(${name}).then(value => {
	${handler}
	}, reason => reason);
	`;
	}

Changing that is outside the scope of this PR.

[domenic]

This doesn't implement the semantics described in https://heycam.github.io/webidl/#es-invoking-callback-functions + https://heycam.github.io/webidl/#es-promise . For that the correct code would be something more like

return new Promise(resolve => resolve(callResult))

Note that Promise.resolve() is inappropriate because it returns its argument in some cases, which is never true for converting to a Web IDL promise type. And note how converting to a Web IDL Promise<T> type ignores the T.

[ExE-Boss]

This code is generated by:

webidl2js/lib/types.js

Lines 330 to 349 in 7d45dfc

	function generatePromise() {
	let handler;
	if (idlType.idlType[0].idlType === "void") {
	// Do nothing.
	handler = "";
	} else {
	const conv = generateTypeConversion(ctx, "value", idlType.idlType[0], [], parentName,
	`${errPrefix} + " promise value"`);
	requires.merge(conv.requires);
	handler = `
	${conv.body}
	return value;
	`;
	}
	str += `
	${name} = Promise.resolve(${name}).then(value => {
	${handler}
	}, reason => reason);
	`;
	}

Changing that is outside the scope of this PR.

[domenic]

Well, I'm not willing to merge this PR if it generates incorrect code that is unusable for consumers of webidl2js, so I guess fixing that is a dependency of this PR then.

[ExE-Boss]

Done in #219.

[domenic]

I'm unable to push commits to this branch again, despite it seemingly being a normal fork :(. I'll merge directly into master from my local copy.

[domenic]

Merged as 4a6e558!