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

Does it make sense for an interface mixin to declare [Default] object toJSON()? #979

Closed
foolip opened this issue Apr 23, 2021 · 3 comments · Fixed by #980
Closed

Does it make sense for an interface mixin to declare [Default] object toJSON()? #979

foolip opened this issue Apr 23, 2021 · 3 comments · Fixed by #980

Comments

@foolip
Copy link
Member

foolip commented Apr 23, 2021

This appears in https://heycam.github.io/webidl/#example-tojson-default-inheritance-and-mixins:

[Exposed=Window]
interface A : B {
  attribute DOMString a;
};

[Exposed=Window]
interface B : C {
  attribute DOMString b;
};

[Exposed=Window]
interface C {
  [Default] object toJSON();
  attribute DOMString c;
};

interface mixin M1 {
  attribute DOMString m1;
};

interface mixin M2 {
  [Default] object toJSON();
  attribute DOMString m2;
};

interface mixin M3 {
  attribute DOMString m3;
};

interface mixin M4 {
  attribute DOMString m4;
};

A includes M1;
A includes M2;
B includes M3;
C includes M4;

Is the use of mixins here in any way significant, or would folding in the mixins result in exactly the same thing?

Put differently, if [Default] object toJSON() isn't scoped to the members of the interface mixin, is there anything special going on here?

Since https://heycam.github.io/webidl/#default-tojson-steps only operates on the inheritance stack, it seems like this example isn't something one would want to do, and doesn't clearly illustrate why "m3" wouldn't be included in the default toJSON() return value. I think that's because B has no [Default] object toJSON(), that "m3" is in a mixin isn't really relevant.

I suspect that this example could be from back when there was implements and "consequential interfaces", when things were more complicated.

If two [Default] object toJSON() operations on the same interface isn't valid, so putting it on a mixin would limit and affect the original interface in an unfortunate way. Maybe it should simply not be allowed?

@domenic
Copy link
Member

domenic commented Apr 23, 2021

Is the use of mixins here in any way significant, or would folding in the mixins result in exactly the same thing?

IMO the intention is that folding them would result in the same thing.

If two [Default] object toJSON() operations on the same interface isn't valid, so putting it on a mixin would limit and affect the original interface in an unfortunate way. Maybe it should simply not be allowed?

I think in general putting two operations with the same name/argument list in an interface, whether via mixins or not, is disallowed?

@foolip
Copy link
Member Author

foolip commented Apr 23, 2021

OK, so it sounds like this example could be made clearer by avoiding mixins, since they're just adding indirection.

Duplicate (not overloaded) operations are indeed not allowed, but I was wondering if like constructors this shouldn't be allowed in partials and mixins at all. But since toJSON is a regular operation that would be a slightly odd carveout. Also stringifiers are kind of similar and explicitly allowed in mixins.

I ran into this when trying to untangle some things in idlharness.js, but my question is now answered.

foolip added a commit to foolip/webidl that referenced this issue Apr 23, 2021
This example was adapted from a previous one in
whatwg#433, but things are much simpler
now and the example makes it seem more complicated than it really is.
Interface mixins aren't special, so make the same point without them.

Fixes whatwg#979.
foolip added a commit to foolip/webidl that referenced this issue Apr 24, 2021
This example was adapted from a previous one in
whatwg#433, but things are much simpler
now and the example makes it seem more complicated than it really is.

Make the points inheritance and mixins separately.

This also fixes the order of attributes on the returned JSON object,
which was wrong the original example. The attributes of the base
interface should be included first. Reverse the inheritance order in
the example to make this clearer, matching `interface B : A` in two
other examples.

Fixes whatwg#979.
@foolip
Copy link
Member Author

foolip commented Apr 24, 2021

I searched for "toJSON" in https://github.com/w3c/webref/tree/master/ed/idl and found that toJSON operations are currently only declared on interfaces, never on partial interfaces or interface mixins. So whatever confusion could occur around this is hypothetical and hasn't actually happened. I'll consider this resolved by #980.

TimothyGu pushed a commit that referenced this issue Apr 27, 2021
This example was adapted from a previous one in
#433, but things are much simpler
now and the example makes it seem more complicated than it really is.

Make the points inheritance and mixins separately.

This also fixes the order of attributes on the returned JSON object,
which was wrong the original example. The attributes of the base
interface should be included first. Reverse the inheritance order in
the example to make this clearer, matching `interface B : A` in two
other examples.

Fixes #979.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants