url: support LF, CR and TAB in pathToFileURL by demurgos · Pull Request #23720 · nodejs/node

demurgos · 2018-10-17T20:08:42Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

devsnek · 2018-10-17T20:29:47Z

this should wait on the resolution of the whatwg issue right?

demurgos · 2018-10-17T20:41:47Z

@devsnek
This is safe to merge already. If whatwg/url#419 is resolved and the pathname setter is modified to not remove newlines and tabs, this will still work. The escaped characters won't be escaped again because % is not escaped by the pathname setter. This PR is forward-compatible.

Resolving the WHATWG issue would allow to have the fix out-of-the box, but I am not sure how long it will take to fix this issue and have it land on Node. Once resolved, my changes wouldn't be necessary anymore and can be reverted in a future commit to simplify the code.

guybedford

Ideally we could tackle the root cause here, so please lets continue to track that, but the temporary fix looks good.

BridgeAR · 2018-10-19T14:54:17Z

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

demurgos · 2018-10-19T16:18:37Z

CI is failing on windows because it internally uses path.resolve, which adds a drive letter.
For example, the testcase /foobar.mjs produces file:///c:/foobar.mjs on Windows (and file:///foobar.mjs on other systems).

Is there a way to get the drive letter for the tests? I think it depends on cwd.

Trott · 2018-11-06T05:56:27Z

Is there a way to get the drive letter for the tests? I think it depends on cwd.

Something like this maybe?

path.parse(process.cwd()).root

If you find you actually want __dirname and not process.cwd(), it would be this:

path.parse(__dirname).root

That will return / on POSIX. If you want something that should return an empty string on POSIX, this clunkier thing will do that:

process.cwd().split(path.sep)[0]
// ...or..
__dirname.split(path.sep)[0]

Trott · 2018-11-20T06:27:34Z

Old CI results have been wiped, so a new CI although I still expect Windows to fail: https://ci.nodejs.org/job/node-test-pull-request/18797/

@demurgos Are you still intending to finish this up? Or would you prefer someone else take on the test adjustments?

Trott · 2018-11-21T03:22:57Z

No surprise, but still failing on Windows:

23:57:43 not ok 487 parallel/test-url-pathtofileurl
23:57:43   ---
23:57:43   duration_ms: 0.221
23:57:43   severity: fail
23:57:43   exitcode: 1
23:57:43   stack: |-
23:57:43     assert.js:86
23:57:43       throw new AssertionError(obj);
23:57:43       ^
23:57:43     
23:57:43     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
23:57:43     + actual - expected
23:57:43     
23:57:43     + 'file:///c:/foobar.mjs'
23:57:43     - 'file:///foobar.mjs'
23:57:43                ^
23:57:43         at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-url-pathtofileurl.js:68:12)
23:57:43         at Module._compile (internal/modules/cjs/loader.js:722:30)
23:57:43         at Object.Module._extensions..js (internal/modules/cjs/loader.js:733:10)
23:57:43         at Module.load (internal/modules/cjs/loader.js:620:32)
23:57:43         at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
23:57:43         at Function.Module._load (internal/modules/cjs/loader.js:552:3)
23:57:43         at Function.Module.runMain (internal/modules/cjs/loader.js:775:12)
23:57:43         at startup (internal/bootstrap/node.js:300:19)
23:57:43         at bootstrapNodeJSCore (internal/bootstrap/node.js:826:3)
23:57:43   ...

guybedford · 2018-11-21T08:34:54Z

Agreed it would be good to get this in regardless of which way the WhatWG decision goes.

demurgos · 2018-11-22T08:19:47Z

I was away for some time. I'll rebase the PR and fix the Windows issue.

I'll use separate test-cases for windows because the resolution logic differs a lot.

Fixes: nodejs#23696

demurgos · 2018-11-22T11:15:11Z

@Trott
I pushed my changes but CI is complaining about my commit message (while it was fine previously).

The following error is thrown:
https://github.com/git-lint/gitlint-parser-base/blob/7c9d06fdd8839517b96eeed70d1337a7676adac4/index.js#L23

Could you also start a full CI?

demurgos · 2018-11-26T16:07:36Z

The CI error about the commit message seems to be global and unrelated. Could someone restart the full CI? @BridgeAR @Trott?

Trott · 2018-11-28T07:09:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18993/ (Currently backlogged so if you get a 404 and it's been less than an hour since I posted this here, please be patient. It should appear once another job somewhere finishes.)

Trott · 2018-11-28T13:58:56Z

CI is green and all other requirements are met, but there were some changes pushed (to accommodate Windows) since the last review. Can one or two folks re-review/reaffirm that this still looks good to them? @BridgeAR @guybedford @TimothyGu @jasnell

demurgos · 2018-12-04T12:01:31Z

@Trott
There was one re-approval. Do you want to wait for more or is it possible to merge this PR?

Fixes: nodejs#23696 PR-URL: nodejs#23720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott · 2018-12-05T23:26:45Z

Landed in ef0c178.

Thanks for the contribution! 🎉

MylesBorins · 2018-12-06T04:37:06Z

added the semver-minor label. Feel free to remove if inaccurate

Fixes: #23696 PR-URL: #23720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. #23708 * The inspection `depth` default is now back at 2. #24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. #23798 * http: * Chosing between the http parser is now possible per runtime flag. #24739 * readline: * The `readline` module now supports async iterators. #23916 * repl: * The multiline history feature is removed. #24804 * tls: * Added min/max protocol version options. #24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. #24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. #23720 * Windows: * Tools are not installed using Boxstarter anymore. #24677 * The install-tools scripts or now included in the dist. #24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. #24655 PR-URL: #24854

Fixes: #23696 PR-URL: #23720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. #23708 * The inspection `depth` default is now back at 2. #24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. #23798 * http: * Chosing between the http parser is now possible per runtime flag. #24739 * readline: * The `readline` module now supports async iterators. #23916 * repl: * The multiline history feature is removed. #24804 * tls: * Added min/max protocol version options. #24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. #24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. #23720 * Windows: * Tools are not installed using Boxstarter anymore. #24677 * The install-tools scripts or now included in the dist. #24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. #24655 PR-URL: #24854

Fixes: nodejs#23696 PR-URL: nodejs#23720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * console,util: * `console` functions now handle symbols as defined in the spec. nodejs#23708 * The inspection `depth` default is now back at 2. nodejs#24326 * dgram,net: * Added ipv6Only option for `net` and `dgram`. nodejs#23798 * http: * Chosing between the http parser is now possible per runtime flag. nodejs#24739 * readline: * The `readline` module now supports async iterators. nodejs#23916 * repl: * The multiline history feature is removed. nodejs#24804 * tls: * Added min/max protocol version options. nodejs#24405 * The X.509 public key info now includes the RSA bit size and the elliptic curve. nodejs#24358 * url: * `pathToFileURL()` now supports LF, CR and TAB. nodejs#23720 * Windows: * Tools are not installed using Boxstarter anymore. nodejs#24677 * The install-tools scripts or now included in the dist. nodejs#24233 * Added new collaborator: * [antsmartian](https://github.com/antsmartian) - Anto Aravinth. nodejs#24655 PR-URL: nodejs#24854

Fixes: #23696 PR-URL: #23720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Oct 17, 2018

demurgos force-pushed the issue-23696 branch from c253fd2 to 64da162 Compare October 17, 2018 20:11

jasnell approved these changes Oct 17, 2018

View reviewed changes

demurgos force-pushed the issue-23696 branch from 64da162 to cd33cec Compare October 17, 2018 20:27

guybedford approved these changes Oct 18, 2018

View reviewed changes

TimothyGu approved these changes Oct 18, 2018

View reviewed changes

BridgeAR approved these changes Oct 19, 2018

View reviewed changes

demurgos force-pushed the issue-23696 branch from cd33cec to 98c141f Compare November 22, 2018 09:36

url: support LF, CR and TAB in pathToFileURL

ce6a0e0

Fixes: nodejs#23696

demurgos force-pushed the issue-23696 branch from 98c141f to ce6a0e0 Compare November 22, 2018 10:53

jasnell approved these changes Nov 30, 2018

View reviewed changes

guybedford approved these changes Dec 4, 2018

View reviewed changes

Trott closed this Dec 5, 2018

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 6, 2018

BridgeAR mentioned this pull request Dec 6, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

Uh oh!

Conversation

demurgos commented Oct 17, 2018

Checklist

Uh oh!

devsnek commented Oct 17, 2018

Uh oh!

demurgos commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 19, 2018

Uh oh!

demurgos commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

Trott commented Nov 21, 2018

Uh oh!

guybedford commented Nov 21, 2018

Uh oh!

demurgos commented Nov 22, 2018

Uh oh!

demurgos commented Nov 22, 2018

Uh oh!

demurgos commented Nov 26, 2018

Uh oh!

Trott commented Nov 28, 2018

Uh oh!

Trott commented Nov 28, 2018

Uh oh!

demurgos commented Dec 4, 2018

Uh oh!

Trott commented Dec 5, 2018

Uh oh!

MylesBorins commented Dec 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

demurgos commented Oct 17, 2018 •

edited

Loading

demurgos commented Oct 19, 2018 •

edited

Loading

Trott commented Nov 6, 2018 •

edited

Loading