Skip to content

activate.fish erases a pre-existing NODE_PATH and never restores it #399

Description

@ekalinin

Summary

Sourcing bin/activate.fish destroys a NODE_PATH the user already had
set. deactivate_node then has nothing to restore, so the variable is gone
for good. NPM_CONFIG_PREFIX and npm_config_prefix have the same defect.

Reproduction

$ nodeenv --prebuilt /tmp/ne
$ fish -c 'set -gx NODE_PATH /my/node_path
           source /tmp/ne/bin/activate.fish
           deactivate_node
           echo "NODE_PATH=[$NODE_PATH]"'
NODE_PATH=[]

Expected NODE_PATH=[/my/node_path], which is what the POSIX bin/activate
does under sh, dash and bash.

Cause

activate.fish calls deactivate_node nondestructive near the top, before
the activation code has saved anything:

# unset irrelevant variables
deactivate_node nondestructive

On that pass _OLD_NODE_PATH is empty, so deactivate_node takes its else
branch and erases the variable:

if test -n "$_OLD_NODE_PATH"
    set -gx NODE_PATH $_OLD_NODE_PATH
    set -e _OLD_NODE_PATH
else
    set -e NODE_PATH
end

The activation code that runs afterwards therefore sees set -q NODE_PATH
as false, never records _OLD_NODE_PATH, and the user's value is lost:

if set -q NODE_PATH
    set -gx _OLD_NODE_PATH $NODE_PATH
    set -gx NODE_PATH "$NODE_VIRTUAL_ENV/lib/node_modules" $NODE_PATH
else
    set -gx NODE_PATH "$NODE_VIRTUAL_ENV/lib/node_modules"
end

b042056 ("fix(nodeenv): guard fish npm config restore") already fixed
exactly this disease, but only for the --isolate-npm variables, by
wrapping the restore in a guard:

# Skip the "deactivate_node nondestructive" pass at the top of
# activate.fish: the variables are only saved after it has run
if set -q NODE_VIRTUAL_ENV

NODE_PATH and the NPM_CONFIG_PREFIX / npm_config_prefix pair never got
that guard.

Scope of the fix

All three variable groups, not just NODE_PATH. The prefix pair is affected
identically; it only looks fine today because the test asserts NODE_PATH
first and stops there.

Test coverage

tests/test_activate_shells.py (added in #397) marks this
xfail(strict=True) as FISH_NODE_PATH_CLOBBER on
test_deactivate_restores_env, via the restoring_shell decorator. When the
fix lands, drop the constant and the decorator and put the test back on
@activating_shell.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions