fix(snippets): update nvm instructions #7467
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Note Your Pull Request seems to be updating Translations of the Node.js Website. Whilst we appreciate your intent; Any Translation update should be done through our Crowdin Project. Thank you! |
There was a problem hiding this comment.
Copilot reviewed 3 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (7)
- apps/site/snippets/en/download/nvm.bash: Language not supported
- apps/site/snippets/fr/download/nvm.bash: Language not supported
- apps/site/snippets/id/download/nvm.bash: Language not supported
- apps/site/snippets/ja/download/nvm.bash: Language not supported
- apps/site/snippets/ko/download/nvm.bash: Language not supported
- apps/site/snippets/uk/download/nvm.bash: Language not supported
- apps/site/snippets/zh-tw/download/nvm.bash: Language not supported
araujogui
changed the title
ljharb
force-pushed
the
update_nvm_instructions
branch
from
5b44aa8 to
a55d025
Compare
|
fwiw it's very confusing that translations exist in version control but shouldn't be updated in version control. |
Well, they are on VC because we need to have them build as part of the source. It might sound confusing, but we use Crowdin for translations :) Crowdin is pretty standard. I don't think it is confusing, of course, it can definitely (and did couple of times) that people will accidentally edit translations on VC and open a PR. That's why we have a Bot, and also a translation guidelines also referenced on README. And on nodejs.org if you click to edit a translated page, it redirects you to Crowdin :) |
| @@ -8,11 +8,13 @@ import { | |||
| import { availableLocaleCodes } from '@/next.locales.mjs'; | |||
There was a problem hiding this comment.
Any reason why changing these files? This doesn't seem to be related? Are you just unhappy with the style?
There was a problem hiding this comment.
Could you maybe revert these changes as these are unrelated?
There was a problem hiding this comment.
I can, but this isn't just about style; using an async function is faster and cleaner than making a promise and returning it.
| @@ -11,7 +11,7 @@ import { availableLocaleCodes } from '../../next.locales.mjs'; | |||
| * This method is used to generate the Node.js Website Download Snippets | |||
There was a problem hiding this comment.
Same here, revert unrelated stylistic changes.
There was a problem hiding this comment.
this file is indeed mostly stylistic, would you prefer a separate PR for it? arrows are intended for inline callbacks, not for the main purpose of a module.
| @@ -8,11 +8,13 @@ import { | |||
| import { availableLocaleCodes } from '@/next.locales.mjs'; | |||
There was a problem hiding this comment.
I can, but this isn't just about style; using an async function is faster and cleaner than making a promise and returning it.
| return import('@/next-data/providers/downloadSnippets').then( | ||
| ({ default: provideDownloadSnippets }) => provideDownloadSnippets(lang)! | ||
| const { default: provideDownloadSnippets } = await import( | ||
| '@/next-data/providers/downloadSnippets' | ||
| ); |
There was a problem hiding this comment.
same comment here - this isn't just style, it's cleaner in the case where it fails to debug the problem.
| @@ -11,7 +11,7 @@ import { availableLocaleCodes } from '../../next.locales.mjs'; | |||
| * This method is used to generate the Node.js Website Download Snippets | |||
There was a problem hiding this comment.
this file is indeed mostly stylistic, would you prefer a separate PR for it? arrows are intended for inline callbacks, not for the main purpose of a module.
Description
I fix the nvm instructions, and also ensure that port 3000 isn't hardcoded.
Validation
Actually tested it locally this time.
Related Issues
Fixes #7434.
Check List
npm run formatto ensure the code follows the style guide.npm run testto check if all tests are passing.npx turbo buildto check if the website builds without errors.