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

Should nvm_auto not prefer current and .nvmrc versions of node to default alias? #2053

Closed
zeorin opened this issue May 31, 2019 · 4 comments
Closed

Comments

@zeorin
Copy link
Contributor

zeorin commented May 31, 2019

Currently, sourcing $NVM_DIR/nvm.sh after running nvm use will change the current version of node to the default alias, if set.
Sourcing $NVM_DIR/nvm.sh in a directory containing an .nvmrc will set the default alias, which I thought was surprising, since it will select the .nvmrc version if there's no default alias set.

The relevant code is:

#nvm.sh (lines 3539-3562)
nvm_auto() {
  nvm_echo "debug: nvm_auto"
  local NVM_MODE
  NVM_MODE="${1-}"
  local VERSION
  if [ "_$NVM_MODE" = '_install' ]; then
    VERSION="$(nvm_alias default 2>/dev/null || nvm_echo)"
    if [ -n "$VERSION" ]; then
      nvm install "$VERSION" >/dev/null
    elif nvm_rc_version >/dev/null 2>&1; then
      nvm install >/dev/null
    fi
  elif [ "_$NVM_MODE" = '_use' ]; then
    VERSION="$(nvm_resolve_local_alias default 2>/dev/null || nvm_echo)"
    if [ -n "$VERSION" ]; then
      nvm use --silent "$VERSION" >/dev/null
    elif nvm_rc_version >/dev/null 2>&1; then
      nvm use --silent >/dev/null
    fi
  elif [ "_$NVM_MODE" != '_none' ]; then
    nvm_err 'Invalid auto mode supplied.'
    return 1
  fi
}

I would expect the auto mode to prefer the current version (if any), then .nvmrc version to the default alias:

 nvm_auto() {
   local NVM_MODE
   NVM_MODE="${1-}"
-  local VERSION
+  local DEFAULT
+  local NVM_CURRENT
   if [ "_$NVM_MODE" = '_install' ]; then
-    VERSION="$(nvm_alias default 2>/dev/null || nvm_echo)"
-    if [ -n "$VERSION" ]; then
-      nvm install "$VERSION" >/dev/null
+    NVM_CURRENT="$(nvm_ls_current 2>/dev/null || nvm_echo)"
+    DEFAULT="$(nvm_alias default 2>/dev/null || nvm_echo)"
+    if [ "_$NVM_CURRENT" != "_$DEFAULT" ] && [ "_$NVM_CURRENT" != "_system" ]; then
+      nvm install "$NVM_CURRENT" >/dev/null
     elif nvm_rc_version >/dev/null 2>&1; then
       nvm install >/dev/null
+    elif [ -n "$DEFAULT" ]; then
+        nvm install "$DEFAULT" >/dev/null
     fi
   elif [ "_$NVM_MODE" = '_use' ]; then
-    VERSION="$(nvm_resolve_local_alias default 2>/dev/null || nvm_echo)"
-    if [ -n "$VERSION" ]; then
-      nvm use --silent "$VERSION" >/dev/null
+    NVM_CURRENT="$(nvm_ls_current 2>/dev/null || nvm_echo)"
+    DEFAULT="$(nvm_resolve_local_alias default 2>/dev/null || nvm_echo)"
+    if [ "_$NVM_CURRENT" != "_$DEFAULT" ] && [ "_$NVM_CURRENT" != "_system" ]; then
+      nvm use --silent "$NVM_CURRENT" >/dev/null
     elif nvm_rc_version >/dev/null 2>&1; then
       nvm use --silent >/dev/null
+    elif [ -n "$DEFAULT" ]; then
+      nvm use --silent "$DEFAULT" >/dev/null
     fi
   elif [ "_$NVM_MODE" != '_none' ]; then
     nvm_err 'Invalid auto mode supplied.'

This way, re-sourcing nvm will prefer the current version (if not system) or the .nvmrc version, or the default alias version.

@ljharb
Copy link
Member

ljharb commented May 31, 2019

.nvmrc is a newer feature; at one point, auto-using selected the default only.

When .nvmrc was added, to avoid breaking auto-use behavior, default remained as the "winner" when both were set.

The current version is never preferred in a new shell - it'd always either be default, or nvmrc.

@zeorin
Copy link
Contributor Author

zeorin commented May 31, 2019

Could we consider enabling the more intuitive (IMO) behaviour I'm suggesting if a flag (new env variable) were set? This would keep backwards compatibility.

@ljharb
Copy link
Member

ljharb commented Jun 1, 2019

That would definitely be back compat - we could also use a sourcing flag - but I’m not sure the complexity is warranted, since you can unset the default and add a ~/.nvmrc containing the version you want as a fallback.

@zeorin
Copy link
Contributor Author

zeorin commented Jun 1, 2019

It wouldn't solve a pain point for me that I haven't already solved in another way. The behaviour just surprised me, thanks for explaining!

@zeorin zeorin closed this as completed Jun 1, 2019
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Dec 6, 2021
Summary:
React-native Xcode build steps (such as "Build JS Bundle") rely on `find.node-sh` to find the correct node binary, using nvm if present. We do this because Xcode may run build steps in a fresh shell environment, presumably for repeatable builds.

This PR fixes `find-node.sh`, to respect any `.nvmrc` file that may be present in the build environment.

Today: `find-node.sh` will set the shell environment to the system node version, and ignores any `.nvmrc` the project may provide to pin node for repeatable builds. By ignoring `.nvmrc`, node versions may differ depending on system environment — between developer laptops, or between developer and CI environments. 😞

This problem has been been noticed before in #8887

### Should this fix happen upstream?

Unfortunately this nvm behavior [is intended](nvm-sh/nvm#2053),  for backwards compatibility

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - find-node.sh now respects .nvmrc

Pull Request resolved: #32712

Test Plan:
Before:
```bash
# nvm isn't loaded
$ which nvm

# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v17.0.1
```

After:
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v16.13.0
```

After (no .nvmrc, should preserve previous behavior):
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ source ./scripts/find-node.sh

$  nvm ls|grep default
default -> v14.17.1

# Expected: v14.17.1
$ node --version
v14.17.1
```

Reviewed By: sota000

Differential Revision: D32889629

Pulled By: ShikaSD

fbshipit-source-id: 527384055e303a87bad43413fb66a7fd117d1a63
arunim2405 pushed a commit to arunim2405/react-native that referenced this issue Jan 23, 2022
Summary:
React-native Xcode build steps (such as "Build JS Bundle") rely on `find.node-sh` to find the correct node binary, using nvm if present. We do this because Xcode may run build steps in a fresh shell environment, presumably for repeatable builds.

This PR fixes `find-node.sh`, to respect any `.nvmrc` file that may be present in the build environment.

Today: `find-node.sh` will set the shell environment to the system node version, and ignores any `.nvmrc` the project may provide to pin node for repeatable builds. By ignoring `.nvmrc`, node versions may differ depending on system environment — between developer laptops, or between developer and CI environments. 😞

This problem has been been noticed before in facebook#8887

### Should this fix happen upstream?

Unfortunately this nvm behavior [is intended](nvm-sh/nvm#2053),  for backwards compatibility

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - find-node.sh now respects .nvmrc

Pull Request resolved: facebook#32712

Test Plan:
Before:
```bash
# nvm isn't loaded
$ which nvm

# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v17.0.1
```

After:
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v16.13.0
```

After (no .nvmrc, should preserve previous behavior):
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ source ./scripts/find-node.sh

$  nvm ls|grep default
default -> v14.17.1

# Expected: v14.17.1
$ node --version
v14.17.1
```

Reviewed By: sota000

Differential Revision: D32889629

Pulled By: ShikaSD

fbshipit-source-id: 527384055e303a87bad43413fb66a7fd117d1a63
kelset pushed a commit to facebook/react-native that referenced this issue Jan 31, 2022
Summary:
React-native Xcode build steps (such as "Build JS Bundle") rely on `find.node-sh` to find the correct node binary, using nvm if present. We do this because Xcode may run build steps in a fresh shell environment, presumably for repeatable builds.

This PR fixes `find-node.sh`, to respect any `.nvmrc` file that may be present in the build environment.

Today: `find-node.sh` will set the shell environment to the system node version, and ignores any `.nvmrc` the project may provide to pin node for repeatable builds. By ignoring `.nvmrc`, node versions may differ depending on system environment — between developer laptops, or between developer and CI environments. 😞

This problem has been been noticed before in #8887

### Should this fix happen upstream?

Unfortunately this nvm behavior [is intended](nvm-sh/nvm#2053),  for backwards compatibility

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - find-node.sh now respects .nvmrc

Pull Request resolved: #32712

Test Plan:
Before:
```bash
# nvm isn't loaded
$ which nvm

# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v17.0.1
```

After:
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ echo v16.13.0 > .nvmrc
$ source ./scripts/find-node.sh

# Expected: v16.13.0
$ node --version
v16.13.0
```

After (no .nvmrc, should preserve previous behavior):
```bash
# we're on default system node
$ which node && node --version
/usr/local/bin/node
v17.0.1

$ source ./scripts/find-node.sh

$  nvm ls|grep default
default -> v14.17.1

# Expected: v14.17.1
$ node --version
v14.17.1
```

Reviewed By: sota000

Differential Revision: D32889629

Pulled By: ShikaSD

fbshipit-source-id: 527384055e303a87bad43413fb66a7fd117d1a63
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

No branches or pull requests

2 participants