Fix mixins, stringifiers, and static attributes #68

TimothyGu · 2017-09-06T03:38:33Z

This requires a change in jsdom to move idlUtils.mixin to jsdom, as it will no longer be needed or included in webidl2js. I could keep it in webidl2js for the time being however, until the next major bump.

Note, this does not yet fix #55

Fixes #49.
Fixes #67.

domenic

Overall looks good; a few suggestions and nits.

What does "Refactor Overloads operation collection" accomplish? (E.g., what would happen with that test case before?)

domenic · 2017-09-08T04:27:24Z

lib/constructs/interface.js

@@ -185,7 +221,7 @@ class Interface {
        definingInterface = member[1];
        continue;
      }
-      if (forbiddenMembers.has(member.name)) {
+      if (forbiddenMembers.has(member.name) || member.name && member.name[0] === "_") {


It's worth adding a check that our IDL parsing library properly strips the _s from source files before handing them to us. I.e., attribute boolean _foo should not trigger an error.

domenic · 2017-09-08T04:28:49Z

test/cases/Static.idl

@@ -0,0 +1,6 @@
+interface Static {
+  static attribute DOMString abc;


Nit: I find the alignment here distracting and would prefer no extra spacing.

domenic · 2017-09-08T04:34:54Z

test/__snapshots__/test.js.snap

@@ -355,6 +355,357 @@ const Impl = require(\\"../implementations/LegacyArrayClass.js\\");
 "
 `;

+exports[`MixedIn.idl 1`] = `


I guess this doesn't directly test that mixing something in to multiple places doesn't reuse the same object... but it does indirectly, as it'd be hard to imagine code generation for the second class that duplicates it.

Sebmaster

Generally: Do we really have to use this sentinel in the inherited member call? Can we just split this up into two "iterators"?

Sebmaster · 2017-09-09T23:32:17Z

lib/constructs/interface.js

        continue;
      }
-      if (forbiddenMembers.has(member.name)) {
+      if (forbiddenMembers.has(member.name) || member.name && member.name[0] === "_") {


Parens this && to reduce (my) confusion about operator precedence.

Sebmaster · 2017-09-09T23:33:49Z

lib/constructs/interface.js

+    }
+
+    // TODO: are these actually allowed by the spec?
+    if (!this.ctx.options.suppressErrors) {


Remove this conditional. suppressErrors should only be used for stuff we know doesn't work but we know is "safe" to ignore.

TimothyGu · 2017-09-10T06:11:24Z

@domenic

Comments addressed.

What does "Refactor Overloads operation collection" accomplish?

It's pure refactoring so that it is closer to the spec's language. No new features added or bugs fixed.

@Sebmaster

Comments addressed.

Do we really have to use this sentinel in the inherited member call? Can we just split this up into two "iterators"?

No, and done.

Sebmaster

Otherwise looks pretty good to me

Sebmaster · 2017-09-12T00:54:22Z

lib/constructs/interface.js

+            if (!this[key].has(member.name)) {
+              this[key].set(member.name, new Operation(this.ctx, this, member));
+            } else {
+              this[key].get(member.name).idls.push(member);


Is the order of these guaranteed? We're sometimes accessing idls[0]. How do we know that that's the correct one? What's in the others?

The order is not guaranteed. Here are the specific cases in which idls[0] is accessed:

utils.isOnInstance(this.idls[0], this.interface.idl)

This checks for [Unforgeable] extended attribute on this.idls[0] and returns true if that is the case. The Web IDL forbids some overloads of an operation having that extended attribute and others of the same operation do not:

If it does appear on an operation, then it must appear on all operations with the same identifier on that interface.

~~We do not enforce this requirement at this moment, and a TODO is included:~~ The check has been added.

webidl2js/lib/constructs/operation.js

Line 28 in 398c67c

// TODO: check [Unforgeable] is applied uniformly.

But assuming the input IDL is well-formed, checking the first overload only should suffice.

this.idls[0].name || this.name

This is a somewhat hacky way of supporting named stringifiers. Basically in case of

stringifier DOMString opName(); DOMString opName(DOMString input);

we create two Operations:

one with op.name being "opName", but op.idls having both overloads, and

the other's op.name property manually overridden with "toString", but op.idls[0] is still "opName", and op.idls.length is always 1.

For the first case, since all overloads have the same name, choosing which one doesn't matter. For the second case, since there is always going to be one overload, the first and only one it is.

OTOH, for a more basic case of

stringifier;

we know there is only always going to be one overload.

this.idls[0].stringifier

~~See above. This can be true only if there is one item in this.idls.~~

This doesn't look necessary any more. Removed.

~~I can add some comments and asserts in the code regarding the last two items if that makes it a bit clearer?~~ Done.

I'm fine with a single comment explaining this here (or at the access points, whatever seems clearer).

Fixes StringifierNamedOperation.idl.

Fixes jsdom#49. Fixes jsdom#67.

TimothyGu · 2017-09-14T00:03:07Z

Just a note when publishing a new version: this is a breaking change as it removes mixin() from the utils file.

domenic · 2017-09-30T13:37:14Z

I've decided that this is not a breaking change because mixin() is undocumented. I think that is the right policy going forward. But, it ended up being done as part of v8.0.0 regardless, so we're safe even if some people don't agree.

In practice, what this means is that fixing #56 and #52 are IMO not breaking changes since they change undocumented APIs and don't change how you write an impl class.

On the other hand I counted #72 as breaking since it changes how you would write an impl class.

TimothyGu force-pushed the mixin-stringifier branch from 4f8bb60 to 1c33dfa Compare September 6, 2017 03:40

TimothyGu mentioned this pull request Sep 6, 2017

Enable more wpt tests from table and anchor categories jsdom/jsdom#1976

Merged

domenic approved these changes Sep 8, 2017

View reviewed changes

Sebmaster requested changes Sep 9, 2017

View reviewed changes

TimothyGu force-pushed the mixin-stringifier branch from 1c33dfa to 398c67c Compare September 10, 2017 06:08

Sebmaster reviewed Sep 12, 2017

View reviewed changes

Sebmaster approved these changes Sep 12, 2017

View reviewed changes

TimothyGu added 4 commits September 12, 2017 09:24

Fix stringifier on a named operation

8e90ad2

Fixes StringifierNamedOperation.idl.

Fix static attrs/ops with same name as normal

c4e6c4c

Refactor Overloads operation collection

7de9dbb

Fix mixin generation

41a0103

Fixes jsdom#49. Fixes jsdom#67.

TimothyGu force-pushed the mixin-stringifier branch from 398c67c to 41a0103 Compare September 12, 2017 01:27

TimothyGu merged commit bf9954d into jsdom:master Sep 12, 2017

TimothyGu deleted the mixin-stringifier branch September 12, 2017 01:30

Zirro mentioned this pull request Sep 20, 2017

Use .webidl extension #73

Merged

ExE-Boss mentioned this pull request Sep 17, 2019

Don't export most things, for mixins #55

Closed

stevedorries pushed a commit to stevedorries/webidl2js that referenced this pull request Jan 27, 2020

fix extattr whitespace error (jsdom#68)

f5d777c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mixins, stringifiers, and static attributes #68

Fix mixins, stringifiers, and static attributes #68

TimothyGu commented Sep 6, 2017 •

edited

Loading

domenic left a comment

domenic Sep 8, 2017

domenic Sep 8, 2017

domenic Sep 8, 2017

Sebmaster left a comment

Sebmaster Sep 9, 2017

Sebmaster Sep 9, 2017

TimothyGu commented Sep 10, 2017

Sebmaster left a comment

Sebmaster Sep 12, 2017

TimothyGu Sep 12, 2017 •

edited

Loading

Sebmaster Sep 12, 2017

TimothyGu commented Sep 14, 2017

domenic commented Sep 30, 2017

		@@ -0,0 +1,6 @@
		interface Static {
		static attribute DOMString abc;

Fix mixins, stringifiers, and static attributes #68

Fix mixins, stringifiers, and static attributes #68

Conversation

TimothyGu commented Sep 6, 2017 • edited Loading

domenic left a comment

Choose a reason for hiding this comment

domenic Sep 8, 2017

Choose a reason for hiding this comment

domenic Sep 8, 2017

Choose a reason for hiding this comment

domenic Sep 8, 2017

Choose a reason for hiding this comment

Sebmaster left a comment

Choose a reason for hiding this comment

Sebmaster Sep 9, 2017

Choose a reason for hiding this comment

Sebmaster Sep 9, 2017

Choose a reason for hiding this comment

TimothyGu commented Sep 10, 2017

Sebmaster left a comment

Choose a reason for hiding this comment

Sebmaster Sep 12, 2017

Choose a reason for hiding this comment

TimothyGu Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Sebmaster Sep 12, 2017

Choose a reason for hiding this comment

TimothyGu commented Sep 14, 2017

domenic commented Sep 30, 2017

TimothyGu commented Sep 6, 2017 •

edited

Loading

TimothyGu Sep 12, 2017 •

edited

Loading