Skip to content

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)

@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
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