Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[v18.x backport] src,lib: reducing C++ calls of esm legacy main resolve #49644

Closed
wants to merge 5 commits into from
Closed

[v18.x backport] src,lib: reducing C++ calls of esm legacy main resolve #49644

wants to merge 5 commits into from

Conversation

H4ad
Copy link
Member

@H4ad H4ad commented Sep 14, 2023

Backport of #48325

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Sep 14, 2023
@H4ad
Copy link
Member Author

H4ad commented Sep 14, 2023

I didn't know how backport works but this PR should be landed with #48664 since they contain fixes that were not detected in this initial PR.

anonrig
anonrig reviewed Sep 14, 2023
View reviewed changes
lib/internal/modules/esm/resolve.js
return guess;
const packageJsonUrlString = packageJSONUrl.href;

if (typeof packageJsonUrlString !== 'string') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is correct? href attribute of a URL always returns string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it always returns a string, but since I didn't know for sure if packageJSONUrl is a URL, in the old code we had validations but in this one I needed to make this validation explicit.

@H4ad H4ad force-pushed the backport-48325-to-v18.x branch 2 times, most recently from 3a1addf to 4f095e1 Compare September 18, 2023 01:37
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 3, 2023 •
edited by ruyadorno
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/54498/
CITGM (v18.x): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3298/
CITGM: https://ci.nodejs.org/job/citgm-smoker/3301/

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Nov 27, 2023

Hey, I'm sorry but a lot of commit landed on the staging branch since you opened this backport and now it has conflicts.

@H4ad
src,lib: reducing C++ calls of esm legacy main resolve
f260c35 
Instead of many C++ calls, now we make only one C++ call
to return a enum number that represents the selected state.

Backport-PR-URL: nodejs#48325
@H4ad
fixup! src,lib: reducing C++ calls of esm legacy main resolve
edbc6b5 
Backport-PR-URL: nodejs#48325
@H4ad
fixup! src,lib: reducing C++ calls of esm legacy main resolve
2c8a182
@H4ad
Copy link
Member Author

H4ad commented Dec 2, 2023

@targos It's okay to include #48664 in this PR since they should land together?

@H4ad H4ad requested a review from targos December 2, 2023 02:45
@targos
Copy link
Member

targos commented Dec 2, 2023

Yes it's ok!

@aduh95 @H4ad
esm: fix emit deprecation on legacy main resolve
a1575a0 
PR-URL: nodejs#48664
Refs: nodejs#48325
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>

Backport-PR-URL: nodejs#48664
@H4ad
Copy link
Member Author

H4ad commented Dec 3, 2023

@targos Done!

@richardlau
Copy link
Member

richardlau commented Jan 12, 2024 •
edited
Loading

@nodejs/lts / @nodejs/releasers Now that Node.js 18 is in maintenance I think the risks of this backport PR outweigh potential benefits. Thoughts?

@ruyadorno
Copy link
Member

@nodejs/lts / @nodejs/releasers Now that Node.js 18 is in maintenance I think the risks of this backport PR outweigh potential benefits. Thoughts?

+1, I don't think we should be landing performance improvement backports in a maintenance release line

@H4ad
Copy link
Member Author

H4ad commented Jan 16, 2024

I agree, this improvement helps but is not that significant that worth the risk.

@H4ad H4ad closed this Jan 16, 2024
@aduh95 aduh95 mentioned this pull request Nov 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig left review comments

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@H4ad @nodejs-github-bot @targos @richardlau @ruyadorno @anonrig @aduh95