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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 1, 2016 •
edited
Loading

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label Dec 3, 2016
@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

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.

@jasnell jasnell self-assigned this Dec 5, 2016
@addaleax
Copy link
Member

addaleax commented Dec 8, 2016

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

@joyeecheung
Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Dec 8, 2016

go for it! :-)

@joyeecheung joyeecheung changed the title test: add test for WHATWG url originFor url, test: WHATWG url originFor should include a base as argument Dec 8, 2016
@joyeecheung
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
Copy link
Member Author

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
Copy link
Member

addaleax commented Dec 8, 2016

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

@joyeecheung
Copy link
Member Author

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
Copy link
Member Author

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

@lpinca
Copy link
Member

lpinca commented Dec 11, 2016

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

@italoacasas
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Dec 12, 2016 via email

@italoacasas
Copy link
Contributor

Landed a84017a

Thanks for the contribution.

italoacasas pushed a commit that referenced this pull request Dec 12, 2016
@joyeecheung @italoacasas
url, test: including base argument in originFor
a84017a 
- 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]>
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
@joyeecheung @Fishrock123
url, test: including base argument in originFor
80cccce 
- 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]>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
italoacasas added a commit that referenced this pull request Dec 15, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
38ecfe0 
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
italoacasas added a commit that referenced this pull request Dec 15, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
617dbf7 
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
italoacasas added a commit that referenced this pull request Dec 17, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
0afa836 
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)
italoacasas added a commit that referenced this pull request Dec 19, 2016
@italoacasas
2016-12-20, Version 7.3.0 (Current)
e46e078 
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)
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 20, 2016
@cjihrig
2016-12-20, Version 7.3.0 (Current)
24a3d0e 
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
cjihrig added a commit to cjihrig/node that referenced this pull request Dec 20, 2016
@cjihrig
2016-12-20, Version 7.3.0 (Current)
ffd01da 
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
cjihrig added a commit that referenced this pull request Dec 20, 2016
@cjihrig
2016-12-20, Version 7.3.0 (Current)
728b61f 
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
Copy link
Contributor

domenic commented Dec 21, 2016

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

@jasnell
Copy link
Member

jasnell commented Dec 21, 2016 via email

@domenic
Copy link
Contributor

domenic commented Dec 21, 2016

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

imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 21, 2016
@imyller
2016-12-20, Version 7.3.0 (Current)
dc0cd98 
    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]>
@joyeecheung joyeecheung deleted the test-for-url-origin-for branch January 2, 2017 05:28
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees

@jasnell jasnell

Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@joyeecheung @jasnell @addaleax @lpinca @italoacasas @domenic @mscdex @MylesBorins @imyller @nodejs-github-bot