Skip to content
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 constructor is missing 'base' option #569

Closed
silverwind opened this issue Aug 18, 2022 · 12 comments
Closed

URL constructor is missing 'base' option #569

silverwind opened this issue Aug 18, 2022 · 12 comments
Assignees

Comments

@silverwind
Copy link
Contributor

silverwind commented Aug 18, 2022

The URL constructor at

constructor(url?: string) {
does not support the second form of the URL constructor which takes a base url:

new URL(url)
new URL(url, base)

Quick test:

# expected
> new URL('', 'https://example.com').pathname
'/'
# actual
> new URL('', 'https://example.com').pathname
''
@silverwind
Copy link
Contributor Author

BTW I think the URL implementation could be sourced from https://github.com/jsdom/whatwg-url, which is pretty much the reference implementation.

@Mas0nShi
Copy link
Contributor

Mas0nShi commented Aug 19, 2022

Hi! @silverwind😊. Thank you for your feedback. jsdom/whatwg-urlso slow because it follows the standard exactly. I think we can test with native module node:url. yes, Im trying do it.

#521
#520 (comment)

@silverwind
Copy link
Contributor Author

silverwind commented Aug 19, 2022

URL in node:url is to my knowledge also a complete copy of jsdom/whatwg-url. If you want to marry happy-dom to node, it might be feasible to use that instead, but it's going to be equally "slow" :)

Oh, any you shouldn't forget URLSearchParams which is also provided by jsdom/whatwg-url.

@Mas0nShi
Copy link
Contributor

Mas0nShi commented Aug 19, 2022

No, I don't approve.
you can test the performance of both.

my test unit.

const wg = require("whatwg-url");

console.time("whatwg-url");
for (let i = 0; i < 1000000; i++) {
	new wg.URL('', 'https://example.com');
}
console.timeEnd("whatwg-url");

console.time("node:url");
for (let i = 0; i < 1000000; i++) {
	new URL('', 'https://example.com');
}
console.timeEnd("node:url");

test parse 1 million times, whatwg-url needs about 11s but node:url only about 1s.

@silverwind
Copy link
Contributor Author

Interesting, they must've done some additional optimizations not present in jsdom/whatwg-url.

@Mas0nShi
Copy link
Contributor

Maybe. I don't look closely, you can see the implementation of node:url at https://github.com/nodejs/node/blob/v18.7.0/lib/url.js

@silverwind
Copy link
Contributor Author

It's in https://github.com/nodejs/node/blob/v18.7.0/lib/internal/url.js. Definitely contains various low-level optimizations. Maybe as a solution, you could check if running on node, use node:url there, otherwise fall back to jsdom/whatwg-url. That would make it compatibly with other JS runtimes as well, like Deno.

@hugo-dlb
Copy link

hugo-dlb commented Oct 10, 2022

After a lot of debugging, I stumbled upon the same issue. In my case I was trying to replace jsdom by happydom for our tests but this is not possible because of this issue. MSW is relying on this constructor (and I'm sure many other libraries do): https://github.com/mswjs/interceptors/blob/main/src/utils/getUrlByRequestOptions.ts#L91

It would be awesome if happy-dom could add this. :)

@shayneo
Copy link

shayneo commented Nov 16, 2022

Found my way here via the vitest 0.25.1 issues. Any new thoughts on supporting the new URL(url, base) constructor? Would a PR be welcome?

@capricorn86
Copy link
Owner

Hi @shayneo! 🙂

Great that you want to contribute. However, I believe that me and @Mas0nShi are already working on a PR related to this, which should solve it.

We will remove the custom URL implementation and replace it with Nodes.

You can see the change here:
https://github.com/capricorn86/happy-dom/pull/520/files#diff-962b53d12390f23f537f3d9b4d96d382fc950b0d4e7464cfefcbf3450a72f0c5R43

@shayneo
Copy link

shayneo commented Nov 16, 2022

Hi @shayneo! 🙂

Great that you want to contribute. However, I believe that me and @Mas0nShi are already working on a PR related to this, which should solve it.

We will remove the custom URL implementation and replace it with Nodes.

You can see the change here: https://github.com/capricorn86/happy-dom/pull/520/files#diff-962b53d12390f23f537f3d9b4d96d382fc950b0d4e7464cfefcbf3450a72f0c5R43

Thanks for the quick reply @capricorn86! Looks like this is well underway.

@capricorn86
Copy link
Owner

We have now fixed this by using the native url module 🙂

You can read more about the release here:
https://github.com/capricorn86/happy-dom/releases/tag/v7.8.0

@capricorn86 capricorn86 self-assigned this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants