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.

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

Read version from use-node-version key of .npmrc #1149

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mcevoypeter
Copy link

@mcevoypeter mcevoypeter commented Oct 20, 2024

Description:
Add support for determining the Node version from the use-node-version key of .npmrc as described in the PNPM docs.

Related issue:
Resolves #1130

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@mcevoypeter mcevoypeter requested a review from a team as a code owner October 20, 2024 17:18
@mcevoypeter
Copy link
Author

@aparnajyothi-y for visibility

@mcevoypeter
Copy link
Author

@aparnajyothi-y , gentle bump on this :)

@johnlindquist
Copy link

We would love to be able to drop nvm entirely and this would be a huge help!

@mcevoypeter
Copy link
Author

@aparnajyothi-y, bumping this again :) I'd love to get this in if my changes look good to you

@GIgako19929
Copy link

[Copilot is generating a summary...] Marge comment

@mcevoypeter
Copy link
Author

@aparnajyothi-y, seems like you're quite busy. Is there someone else I should be seeking a review from?

@kyle-leonhard
Copy link

I'd love to see this too! @aparnajyothi-y or @priya-kinthali, any chance we can nudge you to review this PR?

Copy link

@kyle-leonhard kyle-leonhard left a comment

Choose a reason for hiding this comment

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

LGTM

@kyle-leonhard
Copy link

@aparnajyothi-y or @priya-kinthali, any chance we can nudge you to review this PR? If not, can you suggest another reviewer with bandwidth?

@kyle-leonhard
Copy link

Hi @aparnajyothi-y or @priya-kinthali, could I trouble you to review this PR? I'd greatly appreciate it!

Copy link

@PeterCardenas PeterCardenas left a comment

Choose a reason for hiding this comment

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

seems like the below comments are why maintainers are staying away from approving this pr. i can take another crack in another pr if you don't have time

Choose a reason for hiding this comment

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

maybe instead install @types/ini?

Choose a reason for hiding this comment

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

ini does not have any dependencies so it shouldn't change the package-lock.json i believe

Choose a reason for hiding this comment

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

is it normal that this number of changes are made? seems like quite a red flag

@aparnajyothi-y
Copy link
Contributor

Hello Everyone, Thank you for this pull request. We are looking into it and get back to you once we have some feedback on this :)

@kyle-leonhard
Copy link

kyle-leonhard commented Mar 17, 2025

seems like the below comments are why maintainers are staying away from approving this pr. i can take another crack in another pr if you don't have time

@mcevoypeter - I forked your PR to confirm what @PeterCardenas commented. Change to the lock file and dist directory are much more reasonable: https://github.com/actions/setup-node/pull/1253/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519. All I did was restore the lockfile, add @types/init and rebuild. Feel free to cherry pick the last commit.

Copy link

@BoomchainLabs BoomchainLabs left a comment

Choose a reason for hiding this comment

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

Lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support use-node-version value in .npmrc
8 participants