-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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() { | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It occurred to me that the order of This also makes it possible to support returning the result of |
||
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; | ||
|
||
|
@@ -1712,6 +1819,8 @@ class Interface { | |
} | ||
} | ||
|
||
this.generateLegacyFactoryFunction(); | ||
|
||
this.str += ` | ||
}; | ||
`; | ||
|
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.
@domenic
I decided to pass the
thisValue
as the first parameter, rather thanthis
so that it’s possible to use arrow functions for this:I also decided to include the
new.target
andwrapper
in aprivateData
object to matchconstructor(globalObject, args, privateData)
.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.
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.
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.
We should at the very least include
thisValue
and pass theargs
as an array to leave open the possibility of aprivateData
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.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.
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.