GeezerVison

Review requested:

• @nodejs/url

@nodejs/tsc I've removed the error.input from URL error - ERR_INVALID_URL to make sure we share a similar output with Chrome and Safari. I've also added semver-major label because of the removal of error.input attribute.

Safari returns: TypeError: Type error
Chrome returns: VM78:1 Uncaught TypeError: Failed to construct 'URL': Invalid URL

None of them have the error.input attribute.

I would keep the input, and even add the base, it can very hard to understand what is the error without seeing the input and base

Benchmark CI:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1396/

Could there be a way to get the same performance improvements without removing helpful debugging information? If not, maybe we could use a flag to opt-in to the slower option that outputs the input and base?

Could there be a way to get the same performance improvements without removing helpful debugging information? If not, maybe we could use a flag to opt-in to the slower option that outputs the input and base?

Probably, although I'm not sure. This requires a similar approach to what ThrowAccessDenied is doing in https://github.com/nodejs/node/blob/main/src/permission/permission.cc#L102.

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

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

FWIW, I tend to agree with @aduh95 that removing error information for the sake of performance (on an error path!) does not benefit users. I thought that URL.canParse() was specifically added to avoid having to rely on the performance of the URL error path.

How much is actually the performance hit when keeping the input as an attribute?

@BridgeAR @aduh95 @tniessen I've updated the implementation. Previously this pull request was 142% faster than main. Right now it is 120% faster. I also added base field to the error object.

Appreciate it, if you can review it once again.

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

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

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

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

Hey @BridgeAR, can you re-review this pull request, and remove your block?

I suspect he would still object since the URL isn't part of the message string, what's the cost of adding that back?

I suspect he would still object since the URL isn't part of the message string, what's the cost of adding that back?

I don't follow. URL was never part of the message string? Input is a property of the error object. On top of that I've also added base property to the error as well.

@anonrig ah, I'm probably mixing it up with another PR then, lgtm

Adding to @nodejs/tsc agenda to resolve the block.

There's no need for the TSC to intervene if you addressed the blocking comment

Landed in c829c03