Skip to content

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

Closed
H4ad wants to merge 5 commits intonodejs:v18.x-stagingfrom
H4ad:backport-48325-to-v18.x
Closed

[v18.x backport] src,lib: reducing C++ calls of esm legacy main resolve #49644
H4ad wants to merge 5 commits intonodejs:v18.x-stagingfrom
H4ad:backport-48325-to-v18.x

Conversation

@H4ad
Copy link
Member

@H4ad H4ad commented Sep 14, 2023

Backport of #48325

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 14, 2023

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

@anonrig anonrig Sep 14, 2023

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

@H4ad H4ad Sep 14, 2023

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

nodejs-github-bot commented Oct 4, 2023

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 added 3 commits December 1, 2023 22:49
@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 force-pushed the backport-48325-to-v18.x branch from 4f095e1 to 2c8a182 Compare December 2, 2023 02:09
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>

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

ruyadorno commented Jan 16, 2024

@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
Closed
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