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

Define URL's toJSON() #229

Merged
merged 1 commit into from
Feb 8, 2017
Merged

Define URL's toJSON() #229

merged 1 commit into from
Feb 8, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Feb 2, 2017

This makes it a little easier to use with JSON.stringify().

Fixes #137.

This makes it a little easier to use with JSON.stringify().

Fixes #137.
@annevk annevk mentioned this pull request Feb 2, 2017
@@ -2483,6 +2483,8 @@ interface URL {
attribute USVString search;
[SameObject] readonly attribute URLSearchParams searchParams;
attribute USVString hash;

USVString toJSON();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I'm not a fan of the complicated serializer syntax, I do like the conciseness of just writing

serializer;

which is more parallel to the existing iterable<USVString, USVString>; for URLSearchParams.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be, but this is the way IDL is going for toJSON(). I checked with @bzbarsky and @tobie.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the question one of having serializer; basically be a thing that produces a then-custom-defined toJSON, kinda like stringifier;?

The problem is that unlike stringifier;, where the return value is clear, the return value of toJSON could be all sorts of things. We could define serializer; to induce the equivalent of any toJSON(); but that's more annoying to work with than being able to usefully declare your return type...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I haven't thought of that. How about serializer<USVString>;? It also has the advantage of being more language-independent, while there are precedents for <> meaning "produces" with iterable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? We should be moving away from this baroque, hard-to-understand, "language independent" syntax in favor of simple, do-what-it-says JavaScript-derived syntax.

The only reason iterable is special is because it's a macro that expands to ~4 JS methods. For a simple stand-in for a JS method, we should not create an extra layer of abstraction that requires looking up the Web IDL spec to understand what it does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to assume @TimothyGu is okay with the PR as-is given five days have passed. Hope that's okay. (Still I'll wait a little longer with landing since the test required a further change.)

Copy link
Member

@TimothyGu TimothyGu Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk, sorry about forgetting to reply. I'm okay with the PR in general. The bit about IDL notation is minor and does not affect the semantics anyway.

@@ -2483,6 +2483,8 @@ interface URL {
attribute USVString search;
[SameObject] readonly attribute URLSearchParams searchParams;
attribute USVString hash;

USVString toJSON();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If “URL” interface will be implemented on PHP, should this method be implemented by the name of “jsonSerialize()” instead of “toJSON()”?
https://secure.php.net/manual/class.jsonserializable.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're porting the JavaScript URL class to PHP, you can do whatever you like, as there's no spec for that. It's up to you what you think is most intuitive for your developers (e.g. probably PHP has different naming conventions than JavaScript in many areas, not just JSON serialization).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it. Thank you for your advice.

@annevk
Copy link
Member Author

annevk commented Feb 7, 2017

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

@annevk annevk merged commit 7dcfe5b into master Feb 8, 2017
@annevk annevk deleted the annevk/tojson branch February 8, 2017 09:28
domenic added a commit to jsdom/whatwg-url that referenced this pull request Feb 13, 2017
targos added a commit to targos/node that referenced this pull request Feb 16, 2017
jasnell pushed a commit to nodejs/node that referenced this pull request Feb 16, 2017
PR-URL: #11236
Ref: whatwg/url#229
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
PR-URL: nodejs#11236
Ref: whatwg/url#229
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
italoacasas pushed a commit to nodejs/node that referenced this pull request Feb 22, 2017
PR-URL: #11236
Ref: whatwg/url#229
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants