url: support LF, CR and TAB in pathToFileURL

[demurgos]

Fixes: #23696

Checklist

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

[devsnek]

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

[demurgos]

@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]

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

[demurgos]

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]

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]

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]

No surprise, but still failing on Windows:

23:57:43 not ok 487 parallel/test-url-pathtofileurl23:57:43 ---23:57:43 duration_ms: 0.22123:57:43 severity: fail23:57:43 exitcode: 123:57:43 stack: |-23:57:43 assert.js:8623: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 - expected23: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]

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

[demurgos]

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]

@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]

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

[Trott]

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]

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]

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

[Trott]

Landed in ef0c178.

Thanks for the contribution! 🎉

[MylesBorins]

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