perf(fetch): simplify url serializer

[anonrig]

There is no need for the URLSerializer since we already have a reference to the URL object. url.toString() produces the same result

[codecov-commenter]

Codecov Report

Base: 90.26% // Head: 90.26% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (69852f5) compared to base (a3ce745).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1774      +/-   ##
==========================================
- Coverage   90.26%   90.26%   -0.01%     
==========================================
  Files          58       58              
  Lines        5188     5175      -13     
==========================================
- Hits         4683     4671      -12     
+ Misses        505      504       -1     

Impacted Files	Coverage Δ
lib/fetch/dataURL.js	87.67% <100.00%> (-1.01%)	⬇️
lib/core/connect.js	100.00% <0.00%> (+1.72%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[KhafraDev]

I'm fine with the change (URL's toString implements the url serializer algorithm) but performance of the node core version is almost definitely slower due to its seemingly unnecessary complexity.

https://github.com/nodejs/node/blob/5fad0b93667ffc6e4def52996b9529ac99b26319/lib/internal/url.js#L699

---

Totally unrelated, but removing the empty object from .toString would probably help performance as it would remove the validateObject call.

[anonrig]

If URL's toString method is slow, let's fix it on the Node side. I'm opening a pull request to remove the unnecessary {} from all calls.

[anonrig]

I opened a pull request and started the benchmark CI. We'll have the results in a couple of hours. @KhafraDev

[KhafraDev]

Thanks, I've found an issue with the current undici serialization that I will fix once this is merged as well.

[ronag]

I kind of prefer the literal spec implementation... how much faster is this in practice?

[kibertoad]

@ronag What's the benefit of including complexity within the lib if upstream produces same result? Shouldn't it be safe swap as long as there are tests covering existing behaviour?

[anonrig]

I kind of prefer the literal spec implementation... how much faster is this in practice?

I understand the preference for spec implementation, but new URL is also spec-compliant. The performance is neglectable.

[ronag]

As far as I know URL.toString is not standard and just happens to be similar but could be changed.

[anonrig]

As far as I know URL.toString is not standard and just happens to be similar but could be changed.

Then, I recommend making sure that it's spec compliant in the node core, instead of implementing it twice. I'll be happy to keep track of it. @Trott has more experience on this I believe. What do you think?

[ronag]

I think making URL.toString implement the spec is breaking due to:

if (serialized.endsWith('?')) {
   return serialized.slice(0, -1)
}

I'm pretty sure the echo system relies on that.

[ronag]

URL.href is standardised. Can we base it on that?

[ronag]

The href getter steps and the toJSON() method steps are to return the [serialization](https://url.spec.whatwg.org/#concept-url-serializer) of [this](https://webidl.spec.whatwg.org/#this)’s [URL](https://url.spec.whatwg.org/#concept-url-url).

However, given our

if (serialized.endsWith('?')) {
   return serialized.slice(0, -1)
}

I believe either we or node is implementing URL serialisation incorrectly. @KhafraDev ?

[anonrig]

@KhafraDev @ronag I removed 2 faulty node-fetch tests, which are not spec-compliant, and updated my implementation.

[KhafraDev]

I believe either we or node is implementing URL serialisation incorrectly

Yes, this was the issue I was going to fix. 😄

[KhafraDev]

can you add tests for urls with an empty fragment and an empty search query?

[ronag]

LGTM, although this is more of a refactor than a perf.

[ronag]

CI fails?

[anonrig]

I couldn't find the CI error in the logs, and the tests succeeded locally. Does anybody have more in-depth knowledge?

[KhafraDev]

Looks like

undici/test/fetch/data-uri.js

Lines 52 to 56 in 93f150f

	t.equal(
	URLSerializer(new URL('https://www.google.com/#frag')),
	'https://www.google.com/#frag'
	)

and

undici/test/fetch/data-uri.js

Lines 57 to 61 in 93f150f

	t.equal(
	URLSerializer(new URL('https://www.google.com/#frag'), true),
	'https://www.google.com/'
	)

are failing from the logs.

[KhafraDev]

Suggested change

	const domain = 'https://domain.com#'
	const domain = 'https://domain.com/#'

unless this is expected, and then there's a bug in url serializer?

[anonrig]

I think this is correct. In chrome, the result has / too.

[KhafraDev]

Okay so the last issue hopefully is that you need to call t.end() after the test has run to signify that it's over, or use t.plan(1) to plan a single test. This is only needed when the tests are synchronous, and not very fun to debug from experience. You need to do this for both tests you added.

[anonrig]

I understood what the issue is. If I used a async function in the test callback, I wouldn't need t.plan(1) or t.end(). Thanks for the help @KhafraDev