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

Consider simplifying serializers #188

Closed
bzbarsky opened this issue Oct 15, 2016 · 21 comments · Fixed by #323
Closed

Consider simplifying serializers #188

bzbarsky opened this issue Oct 15, 2016 · 21 comments · Fixed by #323
Assignees

Comments

@bzbarsky
Copy link
Collaborator

I think they're vastly overengineered compared to what people actually want/need in practice. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20361#c4 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=20361#c5 and the fact that there are multiple open issues on the current grammar being buggy, not least because it's too complicated.

@domenic
Copy link
Member

domenic commented Oct 15, 2016

Syntax bikeshed proposal:

toJSON = attributes;
toJSON = own attributes;

It would be really nice to not wonder "WTF is a serializer" whenever I see it :)

@bzbarsky
Copy link
Collaborator Author

Fwiw, what Gecko implements at the moment is this syntax:

jsonifier;

Which leads to a toJSON implementation which does the following:

  1. Allocate an JS object.
  2. Walk from the root of the interface tree to our position in it; for each interface we reach that has a jsonifier on it, stick the corresponding attributes on the JS object. Note that this is sort of like calling your parent's toJSON method, except you've already got the object; this could be rewritten in terms of "find the nearest ancestor that has a jsonifier and call its default toJSON; if there is none allocate an object yourself" or something; the observable behavior is the same.
  3. Return the JS object.

That is, you always do your own attributes, and your ancestor interfaces can opt in to do theirs too. This has been sufficient for all the use cases we've actually encountered, I believe.

@domenic
Copy link
Member

domenic commented Oct 15, 2016

That sounds even better.

@tobie
Copy link
Collaborator

tobie commented Dec 20, 2016

At this point, is it still worth having dedicated syntax?

Couldn't this just be a mixin we'd define in §4. Common definitions?

@annevk
Copy link
Member

annevk commented Dec 20, 2016

A mixin? I'd think you would just define toJSON() for your object.

@tobie
Copy link
Collaborator

tobie commented Dec 20, 2016

Right. I meant for the common case described by @bzbarsky above and for which the proposed syntax is simply jsonifier;.

@tobie
Copy link
Collaborator

tobie commented Dec 21, 2016

See also #260.

@tobie
Copy link
Collaborator

tobie commented Dec 21, 2016

A few questions for @bzbarsky regarding the jsonified proposal above:

  1. How does it deal with circular references, if at all?

  2. What's the precise inheritance story? For example, given the following WebIDL:

    [Constructor]
    interface A {
      attribute DOMString foo;
      jsonified;
    }
    
    interface B : A {
      attribute DOMString bar;
    }

    Which of the following would JSON.stringify(new B()); return?

    { "foo": "", "bar": "" }
    { }

    What about:

    [Constructor]
    interface A {
      attribute DOMString foo;
      jsonified;
    }
    
    interface B : A {
      attribute DOMString bar;
    }
    
    interface C : B {
      attribute DOMString baz;
    }

    Which of the following would JSON.stringify(new C()); return?

    { "foo": "", "bar": "", "baz": "" }
    { "foo": "", "baz": "" }
    { }
  3. What would be the interaction with actual toJSON operations found in the inheritance tree? E.g.:

    [Constructor]
    interface A {
      attribute DOMString foo;
      jsonified;
    }
    
    interface B : A {
      attribute DOMString bar;
      DOMString toJSON();
    }
    
    interface C : B {
      attribute DOMString baz;
      jsonified;
    }

    Imagine toJSON simply returns the value of baz (stringifier = baz in current parlance).
    What happens now for JSON.stringify(new C());?

    { "foo": "", "baz": "" }
    ""
    { "baz": "" }
  4. Could we instead imagine having a default algorithm for toJSON operations (that would return all own and inherited serializable attributes) but that could be overridden in prose?

    [Constructor]
    interface A {
      attribute DOMString foo;
      any toJSON();
    }
    
    interface B : A {
      attribute DOMString bar;
    }
    
    interface C : B {
      attribute DOMString baz;
    }
    
    interface D : C {
      attribute DOMString qux;
      DOMString toJSON();
    }
    
    // D.toJSON() returns the string "Hello world!"

    JSON.stringify(new C()); would return:

    { "foo": "", "bar": "", "baz": "" }

    JSON.stringify(new D()); would return:

    "Hello world!"

    If my understanding of the jsonified proposal is correct, the only feature we'd loose is the ability for interfaces to opt out of having their attributes serilaized. Is this actually useful? It feels a bit weird if serialization requirements start influencing how interfaces inheritance gets split up, imho.

@domenic
Copy link
Member

domenic commented Dec 21, 2016

Talking a bit with @tobie in IRC, I'm hopeful that we can come up with something as simple as "declare your own toJSON method, always". Then you can either go completely custom (using a custom dictionary type, for example, as the return value, or returning a certain string like URL wants to) or you can call some helper abstract op we define in Web IDL that gives you some kind of "serialize all attributes" behavior.

The jsonifier; solution @bzbarsky describes seems a bit too powerful: I would anticipate we want to provide a helper that just always serializes the entire proto chain, and maybe, if there are cases in the platform that want it, one that serializes only own properties. I don't think we need the ability for each link in the inheritance chain to opt-in to whether or not its attributes should be serialized. Do we?


Another issue: the spec doesn't currently seem clear on whether the default serializer observably does a Get(this, "attributeName") or not. It just says:

Let V be the value of the attribute with identifier i.

This seems pretty bogus...

@jan-ivar
Copy link
Contributor

jan-ivar commented Dec 21, 2016

Fyi a case where only specific attributes are serialized: http://w3c.github.io/webrtc-pc/#idl-def-rtcicecandidate

A content script is expected to serialize ice candidates and send them over a signaling channel. Only some of the parameters are needed for this. At the same time, the object has grown other inspection attributes which presumably are useful locally, but need not be sent.

@tobie
Copy link
Collaborator

tobie commented Dec 21, 2016

Fyi a case where only specific attributes are serialized: http://w3c.github.io/webrtc-pc/#idl-def-rtcicecandidate (not saying the reasons behind it necessarily are valid).

Yeah, that would still be possible but require adding a toJSON operation, a dictionary and some prose, e.g.:

interface RTCIceCandidate {
  // members
  RTCIceCandidateJSON toJSON();
};

dictionary RTCIceCandidateJSON {
  DOMString candidate;
  DOMString? sdpMid;
  unsigned short? sdpMLineIndex;
  DOMString ufrag;
};

@tobie
Copy link
Collaborator

tobie commented Dec 22, 2016

@domenic, re:

Another issue: the spec doesn't currently seem clear on whether the default serializer observably does a Get(this, "attributeName") or not. It just says:

Let V be the value of the attribute with identifier i.

This seems pretty bogus...

Currently the spec converts serializable types to serialized values via the serialization behavior, then converts those to ES types via the ES binding. This—I think—explains the above language.

I'm proposing to remove the serialization values altogether and instead simply require that the toJSON operations return a serialization type and let the regular bindings do the conversion to ES.

@domenic
Copy link
Member

domenic commented Dec 22, 2016

My concern is with the default serializer behavior.

@bzbarsky
Copy link
Collaborator Author

How does it deal with circular references, if at all?

However JSON.stringify normally does. All the "jsonifier" thing does is expose a toJSON function that gets called from that algorithm. In practice, circular references throw.

What's the precise inheritance story?

Fwiw, I think I covered this in the description of the algorithm.... ;)

Which of the following would JSON.stringify(new B()); return?

{"foo": "..."}

Well it would if you put a [Constructor] on B . As things stand, it throws on new B().

Which of the following would JSON.stringify(new C()); return?

{"foo": "..."}

A more interesting example is this:

[Constructor]
interface A {
  attribute DOMString foo;
  jsonifier;
}

[Constructor]
interface B : A {
  attribute DOMString foo;
  jsonifier;
}

In this case, the value for "foo" would be the one from B, not from A.

What would be the interaction with actual toJSON operations found in the inheritance tree?

That's a good question. I don't see any way to make these play nice together: toJSON can return primitives, but jsonifier really needs an object to define properties on. In practice, if you want custom serialization behavior on an interface, it doesn't make much sense to define "default" serialization behavior on descendant interfaces. We could just make that invalid IDL.

Could we instead imagine having a default algorithm for toJSON operations
(that would return all own and inherited serializable attributes) but that
could be overridden in prose?

We could imagine it, but it's a bit less useful to me as a UA implementor, because "overridden in prose" is not expressed in IDL, so I have no way to know whether my binding generator should generate and use the default algorithm or not, short of annotating the IDL with that information. At which point we've basically pushed the problem over to one of having [DefaultBehavior] object toJSON() instead of jsonifier or whatnot. Apart from not having the cognitive overhead of extra syntax, what is the win?

JSON.stringify(new C()); would return:

Why would it do any such thing? Why should the toJSON method on A know anything about attributes of descendant interfaces?

Is this actually useful?

Probably not; it's not used in practice in Gecko. It happened to be simple to implement in Gecko: the binding generator for an interface with a jsonifier generates a method to stick the properties on an object, and descendant interfaces can call that method if they want. Your approach means that the binding generator for an interface has to check whether any descendants have a jsonifier and generate the method based on that more-global information. It's doable, but quite a bit more annoying to implement (e.g. in terms of getting build-system dependencies right, etc), which is why I didn't do it that way.

Alternately, we could just forbid interfaces with a jsonifier from having a parent with no jsonifier or something. That adds more coupling between specs if spec X extends an interface from spec Y and wants X to be serializable, but maybe that's ok... Then again, given how W3C specs work maybe this is a terrible idea.

or you can call some helper abstract op we define in Web IDL that gives you some kind of "serialize all attributes" behavior

As I said above, that's probably fine in terms of spec factoring, but may complicate implementation, because that helper may be quite hard to implement generically in an automated way (and half the point here is to have this be automated, so people don't screw up things like adding a new attribute but not adding it to the serializer).

Certainly I'd want a way to express the "use the helper" bit in IDL for Gecko's purposes, but I can just use an extended attribute for that.

I don't think we need the ability for each link in the inheritance chain to opt-in

I agree.

the spec doesn't currently seem clear on whether the default serializer observably does a Get(this, "attributeName") or not

Indeed. Fwiw, in Gecko we call the default getter directly.

@tobie
Copy link
Collaborator

tobie commented Dec 23, 2016

Thanks for the thorough explanation, @bzbarsky. Now I have a better understanding of your proposal. Sounds like we want to treat jsonified and toJSON() as distinct.

Should we add jsonified right away (are there already use cases for it?) or should we just start with toJSON() and add jsonified at a later stage. I'd rather avoid repeating the serializer issue by adding a default serialization mechanism before it becomes a requirement.

@bzbarsky
Copy link
Collaborator Author

are there already use cases for it?

http://searchfox.org/mozilla-central/search?q=jsonifier&path=dom%2Fwebidl

@domenic
Copy link
Member

domenic commented Jan 9, 2017

So one thing I realized might be a problem with the default serializer + custom toJSON approach is that it introduces an inconsistency in whether there's a Get() performed. That is, given the following hypothetical future Web IDL:

interface A {
  attribute DOMString x;
  attribute DOMString y;
  
  toJSON = default; // or whatever, e.g. `jsonifier;` or `[DefaultSerializer] object toJSON();`
};

interface B {
  attribute DOMString x;
  attribute DOMString y;
  
  BJSON toJSON();
};

dictionary BJSON {
  DOMString x;
};

// In prose, define that `B`'s `toJSON()` returns a dictionary initialized
// with the value of `B`'s associated x.

it appears that aInstance.toJSON() will perform two Get()s (for x and y), whereas bInstance.toJSON() will perform zero gets (since it is written in the normal style of all Web IDL operations and accesses "internal slots" or "associated values").

I think we should ensure consistency somehow, besides just requiring spec writers to do manual Get()s. For example:

  • We could provide an abstract operation SerializeToJSON(platformObject, listOfProps) where if listOfProps is omitted it uses all the attributes in the definition. (It does not do property enumeration.) Then we could require every toJSON() method definition to delegate to this.
  • We could use an extended attribute: [Serializer] object toJSON() vs. [Serializer=(x)] object toJSON(). Might get unwieldy for longer cases.
  • We could go with some variation on the existing syntax. Maybe serializer; vs. serializer = { x };.

We also want to handle the case of serializing to the value of a specific attribute, so that url.toJSON() returns url.href. Maybe that's SerializeSinglePropertyToJSON(platformObject, prop) or [AttributeSerializer=prop] or serializer = prop.

This is still simpler than the existing spec in the following respects:

  • Does not allow returning arbitrary mappings, only those derived from attributes
  • Does not allowing opting in or out of inheritance. (The default behavior always includes inherited attributes.)
  • Does not allow named property serialization
  • Does not allow array serialization
  • Does not allow arbitrary serialization behavior defined in prose

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jan 9, 2017

it appears that aInstance.toJSON() will perform two Get()s (for x and y)

Why? See the end of #188 (comment)

@domenic
Copy link
Member

domenic commented Jan 9, 2017

I see; I misunderstood "default getter directly". So that's completely unobservable, right?

I guess that is implementable, and thus probably specable. So we can ignore my mega-post entirely and go back to the simpler plan...

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jan 9, 2017

So that's completely unobservable, right?

Correct.

@tobie tobie self-assigned this Feb 2, 2017
tobie added a commit to tobie/webidl that referenced this issue Feb 27, 2017
* Remove all serializers.
* Add support for JSON stringification through the toJSON operation.
* Add the [Default] extended attribute.
* Add a default JSON operation.

Fixes whatwg#260.
Fixes whatwg#188.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27428.
@tobie
Copy link
Collaborator

tobie commented Mar 2, 2017

tobie added a commit to tobie/webidl that referenced this issue May 11, 2017
* Remove all serializers.
* Add support for JSON stringification through the toJSON operation.
* Add the [Default] extended attribute.
* Add a default JSON operation.

Fixes whatwg#260.
Fixes whatwg#188.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27428.
tobie added a commit that referenced this issue Jun 21, 2017
* Remove all serializers.
* Add support for JSON stringification through the toJSON operation.
* Add the [Default] extended attribute.
* Add a default JSON operation.

Fixes #188.
Closes #259.
Fixes #260.
Fixes #262.
Fixes #370.
Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27428.
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.

5 participants