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

use node version manager #188

Merged
merged 3 commits into from
Nov 25, 2022
Merged

Conversation

krudos
Copy link
Contributor

@krudos krudos commented Nov 21, 2022

No description provided.

Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

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

If you want to add a node version manager, I would prefer n. This would be more less intrusive and easy (use npm to install n)

Dockerfile Outdated Show resolved Hide resolved
@krudos krudos marked this pull request as draft November 22, 2022 09:33
@krudos krudos changed the title use nvm use node version manager Nov 22, 2022
@krudos krudos marked this pull request as ready for review November 22, 2022 09:34
Dockerfile Show resolved Hide resolved
Copy link
Member

@gengjiawen gengjiawen left a comment

Choose a reason for hiding this comment

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

LGTM

@cortinico
Copy link
Member

I would refrain from adding a node version manager in the container frankly. Version managers are user tools. The idea behind a container is to have a fixed set of dependencies that are used to build a given tool.

@gengjiawen gengjiawen mentioned this pull request Nov 23, 2022
@gengjiawen
Copy link
Member

I would refrain from adding a node version manager in the container frankly. Version managers are user tools. The idea behind a container is to have a fixed set of dependencies that are used to build a given tool.

It only added one npm package in the end, the cost is very small. In the long run, I think this is very useful for user-land to do matrix testing(Node.js latest, lts, minimum)

@leotm
Copy link
Contributor

leotm commented Nov 23, 2022

agree w a minimal container but also the small cost of n (or some other hip manager, not talking big o notation)

node 19's out (working for me) and others testing matrices ahead's good for long-term contribution (node 20 next easter)

noticed ubuntu-22.04 comes ootb w: node 18, n and nvm (node 12 removed from all images last wk)

@gengjiawen gengjiawen merged commit 109dfce into react-native-community:master Nov 25, 2022
@gengjiawen
Copy link
Member

Thx for contribution

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.

4 participants