url, test: WHATWG url originFor should include a base as argument#10021

joyeecheung · 2016-12-01T17:39:46Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, url

Description of change

Check if the originFor implementation for WHATWG url parsing is correnct.

Right now this test fails because the implementation is still a WIP.

I believe the API for url.URL.originFor needs changes too. Right now it only takes the url as the only argument, and pass it to the URL constructor if that's a string, but the constructor actually takes two arguments: the url and the base. I put the base as the second argument here since that would not break the test even if the implementation is correct for urls without bases.

cc @jasnell

EDIT: I've fixed the implementation too. This indeed need a base as argument. So now this PR:

Adds tests to check if the originFor implementation for WHATWG url parsing is correnct.
Fixes originFor by including a base as argument

jasnell · 2016-12-05T22:05:05Z

Thank you for this @joyeecheung ... the test is currently failing on master so I think there's an issue to resolve in the URL implementation. I will investigate but it will take a bit of time.

addaleax · 2016-12-08T00:32:49Z

@joyeecheung If you want to, you can probably start digging into this yourself :)

joyeecheung · 2016-12-08T00:40:45Z

@addaleax Yeah looks like he is pretty busy at the moment. @jasnell Mind if I have a try at this?

jasnell · 2016-12-08T01:55:31Z

go for it! :-)

- Add tests to check if the `originFor` implementation for WHATWG url parsing is correnct. - Fix `originFor` by including a base as argument

joyeecheung · 2016-12-08T02:00:46Z

I've managed to fix the implementation(simply adding base as second argument to originFor). Also updated the test because it didn't include unicode serialization and failed the test cases with IDN hosts.

The fix is pushed into this PR too. Can you take a look? @jasnell

addaleax · 2016-12-08T02:14:35Z

Have a CI run: https://ci.nodejs.org/job/node-test-commit/6527/

joyeecheung · 2016-12-08T02:17:16Z

Also I'm not sure about the TupleOrigin#toString(unicode = false). Looking at this I think the default should be true. Firefox behaves this way(ref), but Chrome doesn't at the moment.

joyeecheung · 2016-12-08T11:05:02Z

The failed check looks like a unrelated build failure. Can I have another one?

lpinca · 2016-12-11T20:44:03Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/5358/

italoacasas · 2016-12-12T13:18:06Z

CI is green, before we land, is this semver minor?

jasnell · 2016-12-12T13:37:44Z

The new URL parser is still considered experimental so this can likely land safely as a semver-patch.

…

On Mon, Dec 12, 2016 at 5:18 AM Italo A. Casas ***@***.***> wrote: CI is green, before we land, is this semver minor? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10021 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eW1snwWOxQTTav-qy6ounFPGKw6Qks5rHUmRgaJpZM4LBrox> .

italoacasas · 2016-12-12T14:56:39Z

Landed a84017a

Thanks for the contribution.

- Add tests to check if the `originFor` implementation for WHATWG url parsing is correnct. - Fix `originFor` by including a base as argument PR-URL: #10021 Reviewed-By: James M Snell <[email protected]>

Notable changes: - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2.fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 PR-URL: #10277

Notable changes SEMVER-MINOR - url: - add inspect function to TupleOrigin (Safia Abdalla) #10039 - crypto: - allow adding extra certs to well-known CAs (Sam Roberts) #9139 SEMVER-PATCH - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 - fs: - remove unused argument from copyObject() (Ethan Arrowood) #10041 - timers: - fix handling of cleared immediates (hveldstra) #9759 PR-URL: #10277

Notable changes: * **crypto**: - Allow adding extra certificates to well-known CAs. (Sam Roberts) [#9139](#9139) * **buffer**: - Fix single-character string filling. (Anna Henningsen) [#9837](#9837) - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen) [#9837](#9837) * **url**: - Add inspect function to TupleOrigin. (Safia Abdalla) [#10039](#10039) - Including base argument in originFor. (joyeecheung) [#10021](#10021) - Improve URLSearchParams spec compliance. (Timothy Gu) [#9484](#9484) * **http**: - Remove stale timeout listeners. (Karl Böhlmark) [#9440](#9440) * **build**: - Fix node_g target. (Daniel Bevenius) [#10153](#10153) * **fs**: - Remove unused argument from copyObject(). (EthanArrowood) [#10041](#10041) * **timers**: - Fix handling of cleared immediates. (hveldstra) [#9759](#9759) * **src**: - Add wrapper for process.emitWarning(). (SamRoberts) [#9139](#9139) - Fix string format mistake for 32 bit node.(Alex Newman) [#10082](#10082)

Notable changes: * **crypto**: The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) [#9139](#9139) * **buffer**: buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) [#9837](#9837) * **url**: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) [#10021](#10021) - Improve URLSearchParams to meet specification compliance. (Timothy Gu) [#9484](#9484) * **http**: Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) [#9440](#9440)

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) nodejs#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs#9484 PR-URL: nodejs#10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) #9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) #9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) #9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) #10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) #9484 PR-URL: #10277

domenic · 2016-12-21T04:34:25Z

Wait, what? There is no URL.originFor in the standard. Why are you adding nonstandard methods to URL?

jasnell · 2016-12-21T04:37:01Z

Purely speculative at the moment. The impl here is still considered experimental. This will likely be moved to the url module instead of hanging off the URL object. It's been on my to-do list to get it moved.

…

On Tue, Dec 20, 2016 at 8:34 PM Domenic Denicola ***@***.***> wrote: Wait, what? There is no URL.originFor in the standard. Why are you adding nonstandard methods to URL? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10021 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eRjNYmocnZDiSb3eLVlL-PZjv-eFks5rKKxWgaJpZM4LBrox> .

domenic · 2016-12-21T04:38:41Z

Great. I filed #10374 and #10375 to track removing nonstandard things.

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs/node#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs/node#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Bohlmark) nodejs/node#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs/node#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs/node#9484 PR-URL: nodejs/node#10277 Signed-off-by: Ilkka Myller <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016

imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016

joyeecheung force-pushed the test-for-url-origin-for branch from d636ce0 to 4d17f6dCompare December 1, 2016 17:43

mscdex added the url Issues and PRs related to the legacy built-in url module. label Dec 3, 2016

jasnell self-assigned this Dec 5, 2016

joyeecheung force-pushed the test-for-url-origin-for branch from 4d17f6d to 1c37063Compare December 8, 2016 01:51

joyeecheung changed the title ~~test: add test for WHATWG url originFor~~url, test: WHATWG url originFor should include a base as argumentDec 8, 2016

url, test: WHATWG url originFor should include a base as argument
9c76d3d
- Add tests to check if the `originFor` implementation for WHATWG url parsing is correnct. - Fix `originFor` by including a base as argument

joyeecheung force-pushed the test-for-url-origin-for branch from 1c37063 to 9c76d3dCompare December 8, 2016 01:58

jasnell approved these changes Dec 9, 2016
View reviewed changes

italoacasas closed this Dec 12, 2016

italoacasas mentioned this pull request Dec 15, 2016
v7.3.0 proposal #10277
Merged

joyeecheung deleted the test-for-url-origin-for branch January 2, 2017 05:28

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017

MylesBorins added dont-land-on-v4.x labels Jan 16, 2017

Uh oh!

url, test: WHATWG url originFor should include a base as argument#10021

url, test: WHATWG url originFor should include a base as argument #10021

Uh oh!

Conversation

joyeecheung commented Dec 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Dec 5, 2016

Uh oh!

addaleax commented Dec 8, 2016

Uh oh!

joyeecheung commented Dec 8, 2016

Uh oh!

jasnell commented Dec 8, 2016

Uh oh!

joyeecheung commented Dec 8, 2016

Uh oh!

addaleax commented Dec 8, 2016

Uh oh!

joyeecheung commented Dec 8, 2016

Uh oh!

joyeecheung commented Dec 8, 2016

Uh oh!

lpinca commented Dec 11, 2016

Uh oh!

italoacasas commented Dec 12, 2016

Uh oh!

jasnell commented Dec 12, 2016 via email

Uh oh!

italoacasas commented Dec 12, 2016

Uh oh!

domenic commented Dec 21, 2016

Uh oh!

jasnell commented Dec 21, 2016 via email

Uh oh!

domenic commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

joyeecheung commented Dec 1, 2016•
edited
Loading