-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: simplify legacy resolve functionality #48648
Conversation
3c1be57
to
b3f8342
Compare
13c8363
to
d3cb3e1
Compare
d3cb3e1
to
fe9109b
Compare
@anonrig since this change is a list of general improvements, what do you think about briefly describing the changes in the description and the reasoning behind those? For example:
I think that would make it easier to review this PR because that way I would know what to expect and then check the diff instead of inferring things from the diff directly. |
@RaisinTen you're absolutely right. I updated the pull request description. Appreciate your feedback. |
cc @nodejs/loaders @nodejs/modules |
cc @nodejs/cpp-reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
THROW_ERR_INVALID_FILE_URL_HOST( | ||
env, | ||
"File URL host must be \"localhost\" or empty on %s", | ||
std::string(per_process::metadata.platform)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the std::string()
part needed here?
std::string(per_process::metadata.platform)); | |
per_process::metadata.platform); |
Changes and their reasonings:
nullptr
env->isolate()->ThrowException
with their respective MACROsTHROW_ERR_INVALID_URL_SCHEME
etc.NOLINT
and used pointers to conform the cpp contributing guidelines.legacy_main_extensions
to header file for readability purposes, made itconstexpr
to evaluate it in the compile time.url->get_pathname()
calls which are more costly thanurl->has_empty_hostname()
cc @H4ad