url: use primordials in url.js

[Uzlopak]

Follow up to #54486

@ljharb wrote

@Uzlopak especially for an internal function, in which it could easily be a security issue to provide a hook point for user code - even if it's only ever passed strings (which could also change in the future) the string methods are globally replaceable.

The funny part is that in url.js are many many non primordial function calls.And my question regarding the primordials was regarding my specific example about replacing .charCodeAt(0) with direct accessing the character of a string via index [0].

I refactored it basically to use primordials, when it makes sense.

So e.g. value.slice(i) was transformed to StringPrototypeSlice(value, i) but value.charCodeAt(0) === 58/* : */) was transformed to value[0] === ':', as it makes no sense to me to use StringPrototypeCharCodeAt.

Maybe some method calls are not transformed, which can be changed of course. I did it manually and tbh. I wanted your feedback before I continue to work on this. If you absolutely reject it in principal, than why should I invest more time anyway.

I did not bench it. Maybe somebody can run the benchmarks on the benchmark machine?

[nodejs-github-bot]

Review requested:

• @nodejs/url

[ljharb]

nice!

[ljharb]

should this use your new isFileProtocol helper?

[Uzlopak]

no because isFileProtocol is also accepting ':' at the end of the string but here according to the comment it is about adding ':' at the end of the protocol if it is missing.

[ljharb]

i guess you could use it after the length check

[Uzlopak]

for performance reasons, i think this is better

[codecov]

Codecov Report

Attention: Patch coverage is 95.23810% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.34%. Comparing base (7c76fa0) to head (1f986da).
Report is 10 commits behind head on main.

Files	Patch %	Lines
lib/url.js	95.23%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54489      +/-   ##
==========================================
+ Coverage   87.31%   87.34%   +0.02%     
==========================================
  Files         648      649       +1     
  Lines      182359   182543     +184     
  Branches    34979    35038      +59     
==========================================
+ Hits       159235   159438     +203     
+ Misses      16389    16373      -16     
+ Partials     6735     6732       -3     

Files	Coverage Δ
lib/url.js	97.74% <95.23%> (-0.14%)	⬇️

... and 35 files with indirect coverage changes

[aduh95]

Follow up to #54452

You probably meant another PR, I doubt this is a follow up from the release proposal.

[Uzlopak]

Sry, I meant #54486

[anonrig]

This hasn't been a problem for a really long time. Why are we adding more primordials to this?

[Uzlopak]

because ljharb wrote that they make kind of sense. I personally would like to provide just a PR with performance optimizations, where i basically reduce the amount of used primordials/fuunction calls.

[ljharb]

to be clear, I didn’t suggest making changes - i just said that they always are a good idea for a robust platform in response to your question.

[Uzlopak]

Nope, my question was regarding that special case where

function isIpv6Hostname(hostname) {
  return (
    StringPrototypeCharCodeAt(hostname, 0) === CHAR_LEFT_SQUARE_BRACKET &&
    StringPrototypeCharCodeAt(hostname, hostname.length - 1) ===
    CHAR_RIGHT_SQUARE_BRACKET
  );
}

could be replaced with

function isIpv6Hostname(hostname) {
  return (
   hostname[0] === '[' &&
   hostname[hostname.length - 1] === ']'
  );
}

thus avoiding primordials completely. And you answered, that primordials are always better.

[Uzlopak]

Anyhow. How about reverting those primordials stuff and just focus on the performance improvements?

[ljharb]

I think you misunderstood me then. I said that primordials are always better than instance methods. Obviously it's better to avoid the methods entirely if you can.

[anonrig]

I don't think we should do it. There is no particular reason to pursue more primordials for url right now.