FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

url, test: WHATWG url originFor should include a base as argument by joyeecheung · Pull Request #10021 · nodejs/node · GitHub

/ node Public

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

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:test-for-url-origin-for
ConversationCommits (1)ChecksFiles changed
Closed

url, test: WHATWG url originFor should include a base as argument#10021
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:test-for-url-origin-for

Conversation

joyeecheung commented Dec 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
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 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 4d17f6d Compare 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 commented Dec 5, 2016

Copy link
Copy Markdown
Member

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 self-assigned this Dec 5, 2016

addaleax commented Dec 8, 2016

Copy link
Copy Markdown
Member

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

joyeecheung commented Dec 8, 2016

Copy link
Copy Markdown
Member Author

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

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

jasnell commented Dec 8, 2016

Copy link
Copy Markdown
Member

go for it! :-)

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
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 9c76d3d Compare December 8, 2016 01:58

joyeecheung commented Dec 8, 2016

Copy link
Copy Markdown
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 commented Dec 8, 2016

Copy link
Copy Markdown
Member

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

joyeecheung commented Dec 8, 2016

Copy link
Copy Markdown
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 commented Dec 8, 2016

Copy link
Copy Markdown
Member Author

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

lpinca commented Dec 11, 2016

Copy link
Copy Markdown
Member

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

italoacasas commented Dec 12, 2016

Copy link
Copy Markdown

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

jasnell commented Dec 12, 2016 via email

Copy link
Copy Markdown
Member

italoacasas commented Dec 12, 2016

Copy link
Copy Markdown

Landed a84017a

Thanks for the contribution.

italoacasas pushed a commit that referenced this pull request Dec 12, 2016
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 <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
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 <jasnell@gmail.com>
italoacasas mentioned this pull request Dec 15, 2016
Merged
italoacasas pushed a commit that referenced this pull request Dec 15, 2016
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 pushed a commit that referenced this pull request Dec 15, 2016
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 pushed a commit that referenced this pull request Dec 17, 2016
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 pushed a commit that referenced this pull request Dec 19, 2016
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
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
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
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 commented Dec 21, 2016

Copy link
Copy Markdown
Contributor

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

jasnell commented Dec 21, 2016 via email

Copy link
Copy Markdown
Member

domenic commented Dec 21, 2016

Copy link
Copy Markdown
Contributor

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
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 <ilkka.myller@nodefield.com>
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes

Assignees

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

Back | FazBrowse Home | New Git URL