esm: align sync and async load implementations #49152
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
| if (protocol === 'file:') { | ||
| source = readFileSync(url); | ||
| } else if (protocol === 'data:') { | ||
| const match = RegExpPrototypeExec(DATA_URL_PATTERN, url.pathname); |
There was a problem hiding this comment.
worth pulling this off of url on line 79, in case there's a getter, so as to unconditionally trigger it?
There was a problem hiding this comment.
That seems wasteful, I'm not sure I understand what would be the upside of doing so
There was a problem hiding this comment.
Example benefits:
- a getter that throws will do so at the top of the method, before expensive stuff happens
- algorithm changes won't be as observable - it would otherwise be part of the API of this to only do a Get of "pathname" in this branch, and to only do one total, so refactors could become unintentional breaking changes
- it becomes much clearer what properties are accessed since they're all accessed once, statically, at the top of the method
There was a problem hiding this comment.
If we really care about this, I think we should rather adapt the regex to take the href rather than the pathname.
- a getter that throws will do so at the top of the method, before expensive stuff happens
It's still the first operation after the destructuring in that function.
- algorithm changes won't be as observable - it would otherwise be part of the API of this to only do a Get of "pathname" in this branch, and to only do one total, so refactors could become unintentional breaking changes
You mean if we start needing pathname for other protocols? But a case could be made for the other properties of URL, what if we end up needing search or origin, should we get them as well just in case?
There was a problem hiding this comment.
yep (to be clear, i don't mean get them now just in case, i mean as soon as they're used once in the function, get them unconditionally at the top)
|
What is the goal here? |
Reduce unnecessary URL initializations. |
| * @param {URL} url URL to the module | ||
| * @param {ESModuleContext} context used to decorate error messages | ||
| * @returns {{ responseURL: string, source: string | BufferView }} | ||
| */ | ||
| function getSourceSync(url, context) { |
There was a problem hiding this comment.
| * @param {URL} url URL to the module | |
| * @param {ESModuleContext} context used to decorate error messages | |
| * @returns {{ responseURL: string, source: string | BufferView }} | |
| */ | |
| function getSourceSync(url, context) { | |
| * @param {URL} urlInstance URL to the module | |
| * @param {ESModuleContext} context used to decorate error messages | |
| * @returns {{ responseURL: string, source: string | BufferView }} | |
| */ | |
| function getSourceSync(urlInstance, context) { |
There was a problem hiding this comment.
nit: I think that is not necessary because the code doc already identifies it as type URL, and I think (aside from mistakenly constructing a new instance), the difference likely would not matter because it's castable to a string.
That said, I do sometimes do similar things, like fooRgx, so I'm not opposed to it (just pointing out that info is already available).
There was a problem hiding this comment.
Another thing to consider is that the async implementation also uses url and not urlInstance as a name, and since this PR goal is to align both implementation, that seems to be another reason not to take that suggestion in that PR
There was a problem hiding this comment.
I don't really care, you can feel free to disregard. I suggested it because I think I saw we called this urlInstance somewhere else in the file, so we might want to standardize at some point.
aduh95
added
author ready
github-actions
bot
removed
the
request-ci
lpinca
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 2557932 |
Refs: #48272 PR-URL: #49152 Refs: #47999 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
Refs: #48272
Refs: #47999