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.

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

Implement [LegacyFactoryFunction] #213

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 12 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,17 @@ It is often useful for implementation classes to inherit from each other, if the

However, it is not required! The wrapper classes will have a correct inheritance chain, regardless of the implementation class inheritance chain. Just make sure that, either via inheritance or manual implementation, you implement all of the expected operations and attributes.

### The `[LegacyFactoryFunction]` extended attribute

For interfaces which have the `[LegacyFactoryFunction]` extended attribute, the implementation class file must contain the `legacyFactoryFunction` export, with the signature `(thisValue, globalObject, [...legacyFactoryFunctionArgs], { newTarget, wrapper })`, which is used for:
Copy link
Contributor Author

@ExE-Boss ExE-Boss Mar 20, 2021

Choose a reason for hiding this comment

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

@domenic

I decided to pass the thisValue as the first parameter, rather than this so that it’s possible to use arrow functions for this:

exports.legacyFactoryFunction = (thisValue, globalObject, [...args], { newTarget, wrapper }) => {
	// code
}

I also decided to include the new.target and wrapper in a privateData object to match constructor(globalObject, args, privateData).

Copy link
Member

@domenic domenic Mar 22, 2021

Choose a reason for hiding this comment

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

Would jsdom's implementations of the relevant parts of the HTML spec actually use any of thisValue, newTarget, or wrapper? If not, let's exclude them.

Copy link
Contributor Author

@ExE-Boss ExE-Boss Mar 23, 2021

Choose a reason for hiding this comment

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

We should at the very least include thisValue and pass the args as an array to leave open the possibility of a privateData bag in case https://html.spec.whatwg.org/multipage/media.html#dom-audio, https://html.spec.whatwg.org/multipage/embedded-content.html#dom-image and https://html.spec.whatwg.org/multipage/form-elements.html#dom-option are rewritten.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still feeling like this is over-complex given that there are literally three cases and we know exactly how they behave. Our goal here is not to have a general framework to do any possible LegacyFactoryFunction; it's to implement Option, Audio, and Image.

In particular, I'd like to see these either support return override, or use the passed in thisValue, but not both. I suspect thisValue would be simpler, and perhaps necessary to support class X extends Audio {}. So I suspect the correct signature here will be (thisValue, globalObject, [...args]) with no support for return-override.


- Setting up initial state that will always be used, such as caches or default values
- `thisValue` holds the value of a new uninitialized implementation instance, which may be ignored by returning a different object, similarly to how constructors with overridden return values are implemented.
- Keep a reference to the relevant `globalObject` for later consumption.
- Processing constructor arguments `legacyFactoryFunctionArgs` passed to the legacy factory function constructor, if the legacy factory function takes arguments.
- `newTarget` holds a reference to the value of `new.target` that the legacy factor function was invoked with.
- `wrapper` holds a reference to the uninitialized wrapper instance, just like in `privateData` with the standard impl constructor.

### The init export

In addition to the `implementation` export, for interfaces, your implementation class file can contain an `init` export. This would be a function taking as an argument an instance of the implementation class, and is called when any wrapper/implementation pairs are constructed (such as by the exports of the [generated wrapper module](https://github.com/jsdom/webidl2js#for-interfaces)). In particular, it is called even if they are constructed by [`new()`](newglobalobject), which does not invoke the implementation class constructor.
Expand Down Expand Up @@ -484,6 +495,7 @@ webidl2js is implementing an ever-growing subset of the Web IDL specification. S
- `[Clamp]`
- `[EnforceRange]`
- `[Exposed]`
- `[LegacyFactoryFunction]`
- `[LegacyLenientThis]`
- `[LegacyLenientSetter]`
- `[LegacyNoInterfaceObject]`
Expand All @@ -510,7 +522,6 @@ Notable missing features include:
- `[AllowShared]`
- `[Default]` (for `toJSON()` operations)
- `[Global]`'s various consequences, including the named properties object and `[[SetPrototypeOf]]`
- `[LegacyFactoryFunction]`
- `[LegacyNamespace]`
- `[LegacyTreatNonObjectAsNull]`
- `[SecureContext]`
Expand Down
109 changes: 109 additions & 0 deletions lib/constructs/interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class Interface {
this.attributes = new Map();
this.staticAttributes = new Map();
this.constants = new Map();
this.legacyFactoryFunction = null;

this.indexedGetter = null;
this.indexedSetter = null;
Expand Down Expand Up @@ -391,6 +392,24 @@ class Interface {
throw new Error(msg);
}
}

for (const attr of this.idl.extAttrs) {
if (attr.name === "LegacyFactoryFunction") {
if (!attr.rhs || attr.rhs.type !== "identifier" || !attr.arguments) {
throw new Error(`[LegacyFactoryFunction] must take a named argument list`);
}

if (this.legacyFactoryFunction) {
// This is currently valid, but not used anywhere, and there are plans to disallow it:
// https://github.com/heycam/webidl/issues/878
throw new Error(
`Multiple [LegacyFactoryFunction] definitions are not supported on ${this.name}`
);
}

this.legacyFactoryFunction = attr;
}
}
}

get supportsIndexedProperties() {
Expand Down Expand Up @@ -1644,6 +1663,94 @@ class Interface {
`;
}

generateLegacyFactoryFunction() {
const { legacyFactoryFunction } = this;
if (!legacyFactoryFunction) {
return;
}

const name = legacyFactoryFunction.rhs.value;
if (!name) {
throw new Error(`Internal error: The legacy factory function does not have a name (in interface ${this.name})`);
}

const overloads = Overloads.getEffectiveOverloads("legacy factory function", name, 0, this);
let minOp = overloads[0];
for (let i = 1; i < overloads.length; ++i) {
if (overloads[i].nameList.length < minOp.nameList.length) {
minOp = overloads[i];
}
}

const args = minOp.nameList;
const conversions = Parameters.generateOverloadConversions(
this.ctx,
"legacy factory function",
name,
this,
`Failed to construct '${name}': `
);
this.requires.merge(conversions.requires);

const argsParam = conversions.hasArgs ? "args" : "[]";

this.str += `
function ${name}(${utils.formatArgs(args)}) {
if (new.target === undefined) {
throw new TypeError("Class constructor ${name} cannot be invoked without 'new'");
}

${conversions.body}
`;

// This implements the WebIDL legacy factory function behavior, as well as support for overridding
// the return type, which is used by HTML's element legacy factory functions:
Copy link
Member

Choose a reason for hiding this comment

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

This seems more likely to be a historical artifact of how the spec is written?

It seems like we should be able to support either return overload (like HTML uses for its LegacyFactoryFunctions), or generate a this value (like modern Web IDL-using constructor specs do). We shouldn't need both, I don't think?

In particular given that you can subclass Option per the tests you wrote, I suspect the first line of https://html.spec.whatwg.org/#dom-option is just incorrect.

Copy link
Contributor Author

@ExE-Boss ExE-Boss Mar 23, 2021

Choose a reason for hiding this comment

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

It occurred to me that the order of Impl.legacyFactoryFunction and Impl.init calls is wrong, so I’m fixing that so that the object returned from Impl.legacyFactoryFunction is now the impl, and the result of makeWrapper is what’s returned from the LegacyFactoryFunction constructor.


This also makes it possible to support returning the result of new Impl.implementation.

this.str += `
${this.isLegacyPlatformObj ? "let" : "const"} wrapper = makeWrapper(globalObject, newTarget);
exports._internalSetup(wrapper, globalObject);

const thisValue = Object.create(Impl.implementation.prototype);
const implResult = Impl.legacyFactoryFunction(thisValue, globalObject, ${argsParam});

Object.defineProperty(wrapper, implSymbol, {
value: utils.isObject(implResult) ? implResult : thisValue,
configurable: true
});

`;

if (this.isLegacyPlatformObj) {
this.str += `
wrapper = ${this.needsPerGlobalProxyHandler ?
"makeProxy(wrapper, globalObject)" :
"new Proxy(wrapper, proxyHandler)"};
`;
}

this.str += `

wrapper[implSymbol][utils.wrapperSymbol] = wrapper;
if (Impl.init) {
Impl.init(wrapper[implSymbol]);
}
return wrapper;
}

Object.defineProperty(${name}, "prototype", {
configurable: false,
enumerable: false,
writable: false,
value: ${this.name}.prototype
})

Object.defineProperty(globalObject, "${name}", {
configurable: true,
writable: true,
value: ${name}
});
`;
}

generateInstall() {
const { idl, name } = this;

Expand Down Expand Up @@ -1712,6 +1819,8 @@ class Interface {
}
}

this.generateLegacyFactoryFunction();

this.str += `
};
`;
Expand Down
2 changes: 2 additions & 0 deletions lib/overloads.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ function getOperations(type, A, I) {
case "constructor": {
return I.constructorOperations;
}
case "legacy factory function":
return [I.legacyFactoryFunction];
}
throw new RangeError(`${type}s are not yet supported`);
}
Expand Down
Loading