Skip to content

Commit 1c33dfa

Browse files
TimothyGuTimothyGu
committed
Fix mixin generation
Fixes jsdom#49. Fixes jsdom#67.
1 parent a493730 commit 1c33dfa

File tree

6 files changed

+415
-43
lines changed

6 files changed

+415
-43
lines changed

‎README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ webidl2js is implementing an ever-growing subset of the Web IDL specification. S
297297
- `typedef`s
298298
- Partial interfaces and dictionaries
299299
- Basic types (via [webidl-conversions](https://github.com/jsdom/webidl-conversions))
300-
- Mixins, i.e. `implements` (with a [notable bug](https://github.com/jsdom/webidl2js/issues/49))
300+
- Mixins, i.e. `implements`
301301
- Overload resolution (although [tricky cases are not easy on the implementation class](https://github.com/jsdom/webidl2js/issues/29))
302302
- Variadic arguments
303303
- `[Clamp]`

‎lib/constructs/interface.js

+51-30
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ class Interface {
2626
this.idl = idl;
2727
this.name = idl.name;
2828
this.factory = Boolean(utils.getExtAttr(this.idl.extAttrs, "WebIDL2JSFactory"));
29+
for (const member of this.idl.members) {
30+
member.definingInterface = this.name;
31+
}
2932

3033
this.str = null;
3134
this.opts = opts;
@@ -50,13 +53,13 @@ class Interface {
5053
}
5154

5255
_analyzeMembers() {
53-
let definingInterface = null;
56+
let memberInherited = false;
5457
for (const member of this.inheritedMembers()) {
55-
if (member[0] === inherited) {
56-
definingInterface = member[1];
58+
if (member === inherited) {
59+
memberInherited = true;
5760
continue;
5861
}
59-
if (!definingInterface) {
62+
if (!memberInherited) {
6063
let key;
6164
switch (member.type) {
6265
case "operation":
@@ -92,16 +95,16 @@ class Interface {
9295
case "iterable":
9396
if (this.iterable) {
9497
throw new Error(`Iterable interface ${this.name} inherits from another iterable interface ` +
95-
`${definingInterface}`);
98+
`${member.definingInterface}`);
9699
}
97100
break;
98101
}
99102
}
100103
if (member.type === "operation") {
101104
if (member.getter) {
102105
let msg = `Invalid getter ${member.name ? `"${member.name}" ` : ""}on interface ${this.name}`;
103-
if (definingInterface) {
104-
msg += ` (defined in ${definingInterface})`;
106+
if (member.definingInterface !== this.name) {
107+
msg += ` (defined in ${member.definingInterface})`;
105108
}
106109
msg += ": ";
107110
if (member.arguments.length < 1 ||
@@ -124,8 +127,8 @@ class Interface {
124127
}
125128
if (member.setter) {
126129
let msg = `Invalid setter ${member.name ? `"${member.name}" ` : ""}on interface ${this.name}`;
127-
if (definingInterface) {
128-
msg += ` (defined in ${definingInterface})`;
130+
if (member.definingInterface !== this.name) {
131+
msg += ` (defined in ${member.definingInterface})`;
129132
}
130133
msg += ": ";
131134

@@ -149,8 +152,8 @@ class Interface {
149152
}
150153
if (member.deleter) {
151154
let msg = `Invalid deleter ${member.name ? `"${member.name}" ` : ""}on interface ${this.name}`;
152-
if (definingInterface) {
153-
msg += ` (defined in ${definingInterface})`;
155+
if (member.definingInterface !== this.name) {
156+
msg += ` (defined in ${member.definingInterface})`;
154157
}
155158
msg += ": ";
156159

@@ -168,8 +171,12 @@ class Interface {
168171
}
169172
}
170173
}
171-
if (!definingInterface && member.stringifier) {
172-
const msg = `Invalid stringifier ${member.name ? `"${member.name}" ` : ""}on interface ${this.name}: `;
174+
if (!memberInherited && member.stringifier) {
175+
let msg = `Invalid stringifier ${member.name ? `"${member.name}" ` : ""}on interface ${this.name}`;
176+
if (member.definingInterface !== this.name) {
177+
msg += ` (defined in ${member.definingInterface})`;
178+
}
179+
msg += ": ";
173180
if (member.type === "operation") {
174181
if (!member.idlType) {
175182
member.idlType = { idlType: "DOMString" };
@@ -224,16 +231,16 @@ class Interface {
224231
forbiddenMembers.add(n);
225232
}
226233
}
227-
definingInterface = null;
234+
memberInherited = false;
228235
for (const member of this.inheritedMembers()) {
229-
if (member[0] === inherited) {
230-
definingInterface = member[1];
236+
if (member === inherited) {
237+
memberInherited = true;
231238
continue;
232239
}
233240
if (forbiddenMembers.has(member.name) || member.name && member.name[0] === "_") {
234241
let msg = `${member.name} is forbidden in interface ${this.name}`;
235-
if (definingInterface) {
236-
msg += ` (defined in ${definingInterface})`;
242+
if (member.definingInterface !== this.name) {
243+
msg += ` (defined in ${member.definingInterface})`;
237244
}
238245
throw new Error(msg);
239246
}
@@ -253,6 +260,23 @@ class Interface {
253260
}
254261

255262
implements(source) {
263+
const iface = this.ctx.interfaces.get(source);
264+
if (!iface) {
265+
if (this.ctx.options.suppressErrors) {
266+
return;
267+
}
268+
throw new Error(`${source} interface not found (used as a mixin for ${this.name})`);
269+
}
270+
271+
// TODO: are these actually allowed by the spec?
272+
if (!this.ctx.options.suppressErrors) {
273+
if (iface.idl.inheritance) {
274+
throw new Error(`${source} is used as a mixin for ${this.name}, but it inherits from another interface`);
275+
}
276+
if (iface.mixins.length > 0) {
277+
throw new Error(`${source} is used as a mixin for ${this.name}, but it mixes in other interfaces`);
278+
}
279+
}
256280
this.mixins.push(source);
257281
}
258282

@@ -374,11 +398,12 @@ class Interface {
374398

375399
* inheritedMembers() {
376400
yield* this.idl.members;
377-
for (const iface of [...this.mixins, this.idl.inheritance]) {
378-
if (this.ctx.interfaces.has(iface)) {
379-
yield [inherited, iface];
380-
yield* this.ctx.interfaces.get(iface).inheritedMembers();
381-
}
401+
for (const mixin of this.mixins) {
402+
yield* this.ctx.interfaces.get(mixin).idl.members;
403+
}
404+
if (this.idl.inheritance && this.ctx.interfaces.has(this.idl.inheritance)) {
405+
yield inherited;
406+
yield* this.ctx.interfaces.get(this.idl.inheritance).inheritedMembers();
382407
}
383408
}
384409

@@ -389,11 +414,8 @@ class Interface {
389414
this.requires.add(this.idl.inheritance);
390415
}
391416

392-
if (this.mixins.length !== 0) {
393-
this.requires.addRaw("mixin", "utils.mixin");
394-
for (const mixin of this.mixins) {
395-
this.requires.add(mixin);
396-
}
417+
for (const mixin of this.mixins) {
418+
this.requires.add(mixin);
397419
}
398420

399421
this.str = `
@@ -406,7 +428,6 @@ class Interface {
406428
generateMixins() {
407429
for (const mixin of this.mixins) {
408430
this.str += `
409-
mixin(${this.name}.prototype, ${mixin}.interface.prototype);
410431
${mixin}.mixedInto.push(${this.name});
411432
`;
412433
}
@@ -870,7 +891,7 @@ class Interface {
870891
if (this.supportsNamedProperties && !utils.isGlobal(this.idl)) {
871892
const unforgeable = new Set();
872893
for (const m of this.inheritedMembers()) {
873-
if (m[0] === inherited) {
894+
if (m === inherited) {
874895
continue;
875896
}
876897
if ((m.type === "attribute" || m.type === "operation") && !m.static &&

‎lib/output/utils.js

-12
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,6 @@ function getCopyToBytes(bufferSource) {
2020
return Buffer.from(getReferenceToBytes(bufferSource));
2121
}
2222

23-
function mixin(target, source) {
24-
const keys = Object.getOwnPropertyNames(source);
25-
for (let i = 0; i < keys.length; ++i) {
26-
if (keys[i] in target) {
27-
continue;
28-
}
29-
30-
Object.defineProperty(target, keys[i], Object.getOwnPropertyDescriptor(source, keys[i]));
31-
}
32-
}
33-
3423
const wrapperSymbol = Symbol("wrapper");
3524
const implSymbol = Symbol("impl");
3625
const sameObjectCaches = Symbol("SameObject caches");
@@ -100,7 +89,6 @@ module.exports = exports = {
10089
isObject,
10190
getReferenceToBytes,
10291
getCopyToBytes,
103-
mixin,
10492
wrapperSymbol,
10593
implSymbol,
10694
getSameObject,

0 commit comments

Comments
 (0)