asdf-nodejs

asdf-nodejs copied to clipboard

Node already resolves the correct prefix based on the binary location, the problem with overriding it is that the prefix can be customized to a different location in the .npmrc global config file. We already allow prefix customization by setting the variable itself

Closes #306 Closes #245 Closes #22 Closes #157

@Stratus3D I might need your help here, the variable being overridden was added in the first commit of the project, do you know anything about the motivation of it? I feel like I might be stretching too far from your knowledge right now :sweat_smile:

@augustobmoura I wasn't around when the first commit was made. To be honest this exec-env file has always been a mystery to me. I don't feel like I'm qualified to make a judgement on this, but I'm fine with merging this code.

Would you like me to test these changes on my computer? I'd be happy to exercise this code if you let me know what sequence of steps will validate that things are still working.

Running npm config get prefix before and after the patch should print the same location, if you do not have a prefix configured in .npmrc that is, a use case that we were disallowing because of this variable was overriding it. We were permitting custom prefixes by the variable method but not the config file method

Actually, I noticed something right now, we are setting the prefix to $INSTALL_DIR/.npm, instead of just $INSTALL_DIR. I will try some things out before reopening it

$ npm config get prefix
/Users/<redacted>/.asdf/installs/nodejs/10.24.0/.npm

$ git checkout -b augustobmoura-fix/dont-override-prefix master

$ git pull https://github.com/augustobmoura/asdf-nodejs.git fix/dont-override-prefix
From https://github.com/augustobmoura/asdf-nodejs
 * branch            fix/dont-override-prefix -> FETCH_HEAD
Successfully rebased and updated refs/heads/augustobmoura-fix/dont-override-prefix.

$ npm config get prefix                                                             
/Users/<redacted>/.asdf/installs/nodejs/10.24.0

Looks like it is missing the /.npm segment.

Yeah... I don't think we can remove this file and keep the .npm directory.

Do you know the motivation of the .npm folder? We are actively pointing to it, is not a npm default

@augustobmoura I am not sure. I think @HashNuke wrote the original version of this code.

Please also update list-bin-paths and remove the .npm/bin entry.

With NPM_CONFIG_PREFIX, all global packages are installed under .npm e.g.

/home/foo/.asdf/installs/nodejs/14.17.4
├── .npm
│  ├── bin
│  │  └── grunt -> ../lib/node_modules/grunt/bin/grunt
│  └── lib
│     └── node_modules

Without NPM_CONFIG_PREFIX, global packages end up where they should (directly in the installation folder):

/home/foo/.asdf/installs/nodejs/14.17.4
├── bin
│  ├── grunt -> ../lib/node_modules/grunt/bin/grunt
│  ├── ...other binaries...
...more files...
├── lib
│  └── node_modules
│     ├── grunt
│     └── ...other modules...
...

(check the output of npm root -g to compare)

For upgrading, I would just add a note / echo a message that all global packages need to be re-installed, e.g.

Please reinstall all global packages that are located in the deprecated .npm directory. You can get a list of those packages by executing NPM_CONFIG_PREFIX="$(asdf where nodejs)/.npm" npm list -g

You can delete the .npm folder after you have obtained the listing.

@Stratus3D I'm thinking of just ripping the bandaid off and removing this, as it's being a source of a few issues we had recently and in the past, wdyt?

Go for it @augustobmoura . I've not been paying close attention this repo as asdf-nodejs has been working well for me.

Done :+1: