Use the same parse processing at contractor of URL with setters

[watilde]

Problem this patch fixes

Currently, contractor of URL does not use the same parse processing with the setters in the Class and it makes inconsistent behaviour. See the code below and this patch is to fix the issue.

There is more missing implementation around ASCII encoding at URL and I'm working on it based on this patch. Let me know if you have any question.

Code to reproduce

const url = new URL('http:https://example.com/パス');
console.log(url.pathname);
// => /パス

url.pathname = 'パス';
console.log(url.pathname);
// => /%u30D1%u30B9

[CLAassistant]

All committers have signed the CLA.

[kitsonk]

Why is this necessary? How is this related to the issue mentioned in the PR?

[kitsonk]

Also, this means every instance of URL that does not have protocol, but has baseParts would share the same instance of this._parts which sounds very bad.

[watilde]

In the beginning, I thought the same but turned out setter will refuse provided value if there is no initialized Object in _parts.

[ry]

Could you add a test in js/url_test.ts

[kitsonk]

URL in the browser returns the following, the test should match that:

const url = new URL('http:https://example.com/パス');
console.log(url.pathname);
// => /%E3%83%91%E3%82%B9

url.pathname = 'パス';
console.log(url.pathname);
// => /%E3%83%91%E3%82%B9

[watilde]

@kitsonk That's right. The punycode support is what I'd love to have eventually, but I'd prefer to take a small step to reach out to there. I'm wondering if it's ok to just wrap the official whatwg-url impl, then we do not need to run all WPT tests in this repository that we might need eventually since that module is already checked.

[watilde]

@kitsonk I opened a related issue at #1551. I'd appreciate if we can continue the disscussion at there :)

[ry]

LGTM - thanks

[kitsonk]

@ry, @watilde this still has a problem... This comment was not addressed:

Also, this means every instance of URL that does not have protocol, but has baseParts would share the same instance of this._parts which sounds very bad.

I don't think this should have been merged. I am still missing the purpose of it, because in the constructor, the _parts will always be intiailised. I would have liked to seen a test that replicated the issue before we changed this. All this required was adding pathname to the intialisation object, instead of doing this. Also in the other part, where we intialise each property, upon reflection, it just needed pathname added as well.

Right now every instance of URL which has a basePath passed will share the same instance of parts, so a change to one of them will change them all... For example I bet this will break:

const a = new URL("/bar", "https://example.com/");
const b = new URL("/baz", "https://example.com/");

console.log(a.pathname); // "/baz"
console.log(b.pathname): // "/baz"

a.pathname = "/bar";

console.log(a.pathname); // "/bar"
console.log(b.pathname): // "/bar"

Can we please revert and fix the issue properly?

[ry]

Oops! Sorry @kitsonk I was skimming - revert: #1556