Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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

Open
mcevoypeter wants to merge 3 commits into actions:main
base: main
Choose a base branch
Loading
from mcevoypeter:issue/1130

Conversation

Copy link

@mcevoypeter mcevoypeter commented Oct 20, 2024 •
edited
Loading

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.

tordans, mattlewis92, johnlindquist, ikenox, k-taro56, takayukiooi, kyle-leonhard, renoinn, yuheiy, and PeterCardenas reacted with thumbs up emoji
@mcevoypeter mcevoypeter requested a review from a team as a code owner October 20, 2024 17:18
Copy link
Author

@aparnajyothi-y for visibility

Copy link
Author

@aparnajyothi-y , gentle bump on this :)

Copy link

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

Copy link
Author

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

Copy link
Author

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

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

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?

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 •
edited
Loading

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

Copy link

@PeterCardenas PeterCardenas Mar 16, 2025

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?

Copy link

@PeterCardenas PeterCardenas Mar 16, 2025

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

Copy link

@PeterCardenas PeterCardenas Mar 16, 2025

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

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 :)
PeterCardenas and kyle-leonhard reacted with heart emoji

Copy link

kyle-leonhard commented Mar 17, 2025 •
edited
Loading

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.
PeterCardenas reacted with hooray emoji PeterCardenas reacted with heart emoji

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

Copy link

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 :)

hi @aparnajyothi-y, I had a little time today so I went ahead and fixed up a few of the issue with this PR and created: #1253. Please take a look when you have a chance!
PeterCardenas reacted with heart emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

4 more reviewers

@PeterCardenas PeterCardenas PeterCardenas left review comments

@kyle-leonhard kyle-leonhard kyle-leonhard approved these changes

@greyaxie greyaxie greyaxie approved these changes

@BoomchainLabs BoomchainLabs BoomchainLabs approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support use-node-version value in .npmrc

AltStyle によって変換されたページ (->オリジナル) /