lib: reduce amount of caught URL errors #52658
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
anonrig
force-pushed
the
reduce-url-throws
branch
from
a66a1c1
to
e561d99
Compare
anonrig
force-pushed
the
reduce-url-throws
branch
2 times, most recently
from
7bc1c6e
to
9cfdd53
Compare
anonrig
force-pushed
the
reduce-url-throws
branch
from
9cfdd53
to
b0f5fed
Compare
| } catch { | ||
| // responseURLObj not defined will throw in next branch. | ||
| } | ||
| responseURLObj = URL.parse(responseURL); |
There was a problem hiding this comment.
Do we have access to the Ada binding directly? If so, we might prefer that as it can't be affected by user mutations
There was a problem hiding this comment.
Another option is to extract the static method as an internal function and use it.
There was a problem hiding this comment.
if I export URLParse: URL.parse in internal/url.js would it make it immutable?
There was a problem hiding this comment.
From what I can see in the implementation code, it seems we could simply pass kParseURLSymbol as a third argument, although URLParse is probably better for readability
| resolved = URL.parse(specifier, base); | ||
|
|
||
| if (resolved == null) { | ||
| throw new ERR_UNSUPPORTED_RESOLVE_REQUEST(specifier, base); |
There was a problem hiding this comment.
I'm not sure about this one, it seems useful to have the error as cause (also if we're throwing an error anyway, I don't think we need to worry too much about performance)
There was a problem hiding this comment.
Throwing an error, catching it, and later throwing it again has a really big performance impact. I'm not quite sure about the un-happy path probability of this code...
There was a problem hiding this comment.
I don't think "passing invalid URL to import()" is a use-case we want to support.
Throwing an error, catching it, and later throwing it again has a really big performance impact
You say "really big", but I'm unconvinced we could even measure it.
There was a problem hiding this comment.
While I'm generally happy with the revised approach (creating one error is better than creating two)... Unfortunately I think dropping the cause ends up making this a semver-major change. While unlikely, I can imagine code in the wild that looks at the ERR_UNSUPPORTED_RESOLVE_REQUEST error then looks for the cause to determine exactly why.
| const urlPath = URL.parse(url); | ||
|
|
||
| if (urlPath != null) { | ||
| try { | ||
| // We still need to read the FS to detect the exports. | ||
| source ??= readFileSync(urlPath, 'utf8'); | ||
| } catch { | ||
| // Continue regardless of error. | ||
| } |
There was a problem hiding this comment.
Same here, also I don't think URL.parse(url) can ever be null at this point
There was a problem hiding this comment.
Any downside to have this check either way?
There was a problem hiding this comment.
Code readability, noise in the git blame.
| } catch { | ||
| // responseURLObj not defined will throw in next branch. | ||
| } | ||
| responseURLObj = URL.parse(responseURL); |
There was a problem hiding this comment.
This is mutable. I am pretty sure at least in the ESM loader we try not to look things up from mutable public APIs…
There was a problem hiding this comment.
if I export URLParse: URL.parse in internal/url.js would it make it immutable?
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
| resolved = URL.parse(specifier); | ||
|
|
||
| if (resolved == null && isRemote && !BuiltinModule.canBeRequiredWithoutScheme(specifier)) { | ||
| throw new ERR_UNSUPPORTED_RESOLVE_REQUEST(specifier, base); |
There was a problem hiding this comment.
Likewise here... I'm afraid that dropping the cause is probably semver-major
Since we have
URL.parse()now, we don't need try/catch withnew URL(). This will potentially improve the performance of hot paths.cc @nodejs/url