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

download: fix nvm instructions #7434

Merged
merged 1 commit into from
Feb 1, 2025
Merged

download: fix nvm instructions #7434

merged 1 commit into from
Feb 1, 2025

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jan 30, 2025

Description

add "source nvm.sh" to the instructions

Validation

because this is how nvm works

Related Issues

Fixes #7433

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@Copilot Copilot bot review requested due to automatic review settings January 30, 2025 16:53
@ljharb ljharb requested a review from a team as a code owner January 30, 2025 16:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • apps/site/snippets/en/download/nvm.bash: Language not supported

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Jan 31, 2025 7:03am

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 99 🟢 100 🟢 96 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 90%
88.71% (739/833) 75.94% (240/316) 87.65% (142/162)

Unit Test Report

Tests Skipped Failures Errors Time
182 0 💤 0 ❌ 0 🔥 5.674s ⏱️

@bmuenzenmeyer bmuenzenmeyer added the fast-track Fast Tracking PRs label Jan 31, 2025
@bmuenzenmeyer
Copy link
Collaborator

i'd like to fast track this before the weekend - any concerns?

@ovflowd ovflowd merged commit 72131a7 into main Feb 1, 2025
22 of 23 checks passed
@ovflowd ovflowd deleted the update_nvm_instructions branch February 1, 2025 14:53
@MikeMcC399
Copy link
Contributor

@ovflowd

I'm now getting a 500 Internal Server Error

https://nodejs.org/en/download
selecting Linux / nvm

Will this sort itself out or does it need intervention?

@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2025

@ljharb you broke the downloads page haha -- Did you test this locally before making a PR?

ovflowd added a commit that referenced this pull request Feb 1, 2025
@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2025

Ive reverted these changes.

@ljharb
Copy link
Member Author

ljharb commented Feb 1, 2025

I’m confused how i could have; isn’t this file just text that’s inlined into the page? I indeed didn’t test it locally :-/

I’ll get another PR ready and I’ll test it locally first this time.

@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2025

I’m confused how i could have; isn’t this file just text that’s inlined into the page? I indeed didn’t test it locally :-/

I’ll get another PR ready and I’ll test it locally first this time.

It is because of the ${} -- acorn will handle that as a variable

The whole contents of these bash files get parsed as if they are template strings. So you need to escape the $ I think.

@ljharb
Copy link
Member Author

ljharb commented Feb 1, 2025

ahhh ok thanks, that makes sense. I’ll try to add a ci test too that would have caught this.

@bmuenzenmeyer
Copy link
Collaborator

I indeed didn’t test it locally :-/

neither did I - lesson learned! more evidence for #7395

ljharb added a commit that referenced this pull request Feb 3, 2025
@MikeMcC399
Copy link
Contributor

@bmuenzenmeyer

I indeed didn’t test it locally :-/

neither did I - lesson learned! more evidence for #7395

Thanks to all of you who are involved here!

ljharb added a commit that referenced this pull request Feb 3, 2025
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
* fix: don’t hardcode 3000; use the actual port

* download: fix nvm instructions (#7434)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track Fast Tracking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nvm download snippet fails on first run
5 participants