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

url: support LF, CR and TAB in pathToFileURL #23720

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@demurgos
Copy link
Contributor

demurgos commented Oct 17, 2018

Fixes: #23696

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

@demurgos demurgos force-pushed the demurgos:issue-23696 branch from c253fd2 to 64da162 Oct 17, 2018

@demurgos demurgos force-pushed the demurgos:issue-23696 branch from 64da162 to cd33cec Oct 17, 2018

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Oct 17, 2018

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

@demurgos

This comment has been minimized.

Copy link
Contributor

demurgos commented Oct 17, 2018

@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
Copy link
Contributor

guybedford left a comment

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

@BridgeAR

This comment has been minimized.

@demurgos

This comment has been minimized.

Copy link
Contributor

demurgos commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Member

Trott commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Member

Trott commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

Trott commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 21, 2018

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

@demurgos

This comment has been minimized.

Copy link
Contributor

demurgos commented Nov 22, 2018

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.

@demurgos demurgos force-pushed the demurgos:issue-23696 branch from cd33cec to 98c141f Nov 22, 2018

@demurgos demurgos force-pushed the demurgos:issue-23696 branch from 98c141f to ce6a0e0 Nov 22, 2018

@demurgos

This comment has been minimized.

Copy link
Contributor

demurgos commented Nov 22, 2018

@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

This comment has been minimized.

Copy link
Contributor

demurgos commented Nov 26, 2018

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

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 28, 2018

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

This comment has been minimized.

Copy link
Member

Trott commented Nov 28, 2018

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

This comment has been minimized.

Copy link
Contributor

demurgos commented Dec 4, 2018

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 5, 2018

url: support LF, CR and TAB in pathToFileURL
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

This comment has been minimized.

Copy link
Member

Trott commented Dec 5, 2018

Landed in ef0c178.

Thanks for the contribution! 🎉

@Trott Trott closed this Dec 5, 2018

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 6, 2018

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

BridgeAR added a commit that referenced this pull request Dec 6, 2018

url: support LF, CR and TAB in pathToFileURL
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>

BridgeAR added a commit that referenced this pull request Dec 6, 2018

2018-12-06, Version 11.4.0 (Current)
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

@BridgeAR BridgeAR referenced this pull request Dec 6, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 7, 2018

url: support LF, CR and TAB in pathToFileURL
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>

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
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

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
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

BridgeAR added a commit that referenced this pull request Dec 7, 2018

url: support LF, CR and TAB in pathToFileURL
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>

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
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

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
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

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

url: support LF, CR and TAB in pathToFileURL
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>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment