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

toJSON() serialization #137

Closed
stevenvachon opened this issue Aug 8, 2016 · 22 comments
Closed

toJSON() serialization #137

stevenvachon opened this issue Aug 8, 2016 · 22 comments

Comments

@stevenvachon
Copy link

I think it'd be handy if all URL instances would serialize to the value of href.

@annevk
Copy link
Member

annevk commented Aug 8, 2016

toString() does that. For toJSON() I can imagine folks might want to see the individual components too, so I'm not really sure it makes sense to define a default there.

@stevenvachon
Copy link
Author

The individual components don't show up with JSON.stringify() because they are properties of prototype.

@annevk
Copy link
Member

annevk commented Aug 8, 2016

I think you're referring to something Chrome used to do at some point, not the standard way it's supposed to work.

@Sebmaster
Copy link

@annevk Are you saying the spec shouldn't define a default and should just always return "{}", or are you saying that this should already work differently (i.e. list all getters)?

@stevenvachon
Copy link
Author

stevenvachon commented Aug 8, 2016

Firefox v48 stringifies all properties/getters, but whatwg-url does not.

@domenic
Copy link
Member

domenic commented Aug 8, 2016

Then Firefox is not following the spec for JSON.stringify. I suspect something else is going on.

@stevenvachon
Copy link
Author

stevenvachon commented Aug 8, 2016

Chrome v52 also results in {}.

@domenic that's not what @annevk has said.

@domenic
Copy link
Member

domenic commented Aug 8, 2016

Yes. I think @annevk is confused. The JSON.stringify spec is unambiguous.

@stevenvachon
Copy link
Author

console.log( JSON.stringify(new Date()) )

produces:

"2016-08-08T23:29:58.183Z"

@annevk
Copy link
Member

annevk commented Aug 9, 2016

I didn't say that Firefox stringifies all properties/getters?

All I was saying that we could define something here, but it's not clear to me what the best option is.

@stevenvachon stevenvachon reopened this Aug 9, 2016
@jasnell
Copy link
Collaborator

jasnell commented Aug 9, 2016

Having a URL.prototype.toJSON() that essentially just returned URL.prototype.href would likely be the best option given that it's likely what most would expect or need.

@annevk
Copy link
Member

annevk commented Oct 19, 2016

If .toJSON() returns .href you might as well write .href, no? Where would this come up as being useful?

@stevenvachon
Copy link
Author

@annevk when serializing the URL indirectly, as part of another object:

JSON.stringify({ key: new URL("http://domain/") });

@annevk
Copy link
Member

annevk commented Oct 19, 2016

@tobie would we just define toJSON for this? I saw IDL might change around this.

@tobie
Copy link
Contributor

tobie commented Oct 19, 2016

@annevk right, that's being discussed in whatwg/webidl#188. Haven't looked into it myself yet.

@domenic
Copy link
Member

domenic commented Oct 19, 2016

Apparently it is currently disallowed to create an operation named toJSON:

The identifier of any of the abovementioned IDL constructs must not be “constructor”, “toString”, “toJSON”, or begin with a U+005F LOW LINE ("_") character. These are known as reserved identifiers.

So I guess the way to currently do this would be to just define serializer; and have prose say that it returns the same value as running the href algorithm. We could update it as Web IDL gets its serializer business together.

@bzbarsky
Copy link

In terms of the current IDL spec, you can write:

serializer = href;

to get the desired behavior, no? But as mentioned in whatwg/webidl#188 I think the current spec is way overdesigned.

annevk added a commit to whatwg/webidl that referenced this issue Feb 2, 2017
annevk added a commit to whatwg/webidl that referenced this issue Feb 2, 2017
bzbarsky pushed a commit to whatwg/webidl that referenced this issue Feb 2, 2017
annevk added a commit that referenced this issue Feb 2, 2017
This makes it a little easier to use with JSON.stringify().

Fixes #137.
@annevk
Copy link
Member

annevk commented Feb 2, 2017

I created a PR for this, #229. Please review! Will write a test too.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 2, 2017
@styfle
Copy link

styfle commented Feb 2, 2017

I agree with @stevenvachon. As an end user, I would expect a URL to serialize to the full href string the same way Date serializes to a full ISO string.

During deserialization, the user can pass the string into the URL() constructor the same way you pass the ISO string into the Date() constructor.

Here's an example:

var data = {
  theUrl: new URL("https://example.com/foo"),
  theDate: new Date("2017-02-02T17:06:41.229Z")
};

var str = JSON.stringify(data);
// "{"theUrl":"https://example.com/foo","theDate":"2017-02-02T17:06:41.229Z"}"

var clone = JSON.parse(str);

new Date(clone.theDate).valueOf() === data.theDate.valueOf(); // Current this is true
new URL(clone.theUrl).href === data.theUrl.href; // I would expect this to be true

Note that the Date does not stringify as {fullYear: 2017, month: 1, date: 2}. Neither should the URL.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 8, 2017
annevk added a commit that referenced this issue Feb 8, 2017
This makes it a little easier to use with JSON.stringify().

Tests: web-platform-tests/wpt#4702.

Fixes #137.
@annevk
Copy link
Member

annevk commented Feb 8, 2017

Browser bugs (none for Edge as it doesn't implement the API yet):

@annevk
Copy link
Member

annevk commented Feb 8, 2017

It was pointed out to me Edge does implement the API! Oops. I filed https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10867624/ on Edge.

@styfle
Copy link

styfle commented Mar 1, 2017

This just landed in node.js 7.7.0 today 🎉
That was quick! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants