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

Put local node_modules/.bin in PATH, fixes #22 #25

Merged
merged 1 commit into from
Jul 8, 2016

Conversation

CMTegner
Copy link
Contributor

A lot of back and forth eventually led to this relatively simple fix. By forcing rackt-cli/node_modules/.bin to always be in PATH before executing any of the scripts, you don't have to do any manual work trying to figure out where the binaries are located.

This is how it works:

  1. For global installs you don't go via npm, so you don't get the automatic module lookup PATH it constructs, hence the need to manually put node_modules/.bin in PATH. Prior to this fix it worked by manually creating paths to the various binaries in node_modules/.bin. Now we just reference the binaries indirectly and let the shell figure out where they are by looking in PATH. Nothing has changed for global installs.
  2. For dependency installs we are in a npm context, so we know the parent node_modules dir is in PATH, but since we're not running bin/rackt via npm ourselves we manually prepend rackt-cli/node_modules/.bin to path to ensure that the shell looks there first for binaries, before looking in the parent's node_modules/.bin. Prior to this fix we did the same thing as above, but since the dependency paths vary based on sibling dependencies we can't reliably create paths without doing a lot of tedious path traversal. Dependency installs now work on both npm 2 and 3.

I've tried as best I can to test the various scenarios, but any additional testing would be appreciated. I currently only know of one failing case (which also failed prior to this fix): Referencing bin/rackt directly when installed as a dependency will fail, due to the parent node_modules missing from PATH. Using it via npm run-script will of course work.

@mzabriskie mzabriskie merged commit ba32ae1 into mzabriskie:master Jul 8, 2016
@mzabriskie mzabriskie mentioned this pull request Jul 8, 2016
@CMTegner
Copy link
Contributor Author

CMTegner commented Jul 9, 2016

Great, thanks!

@mzabriskie
Copy link
Owner

Running rackt test now fails as of this PR as karma cannot find webpack or sourcemap preprocessors.

@CMTegner
Copy link
Contributor Author

CMTegner commented Jul 9, 2016

Which version of node/npm are you on?

christian on dante in ~/git/rackt-cli on master*
› node --version
v6.0.0
christian on dante in ~/git/rackt-cli on master*
› npm --version
3.8.6
christian on dante in ~/git/rackt-cli on master*
› rackt version
0.6.0
christian on dante in ~/git/rackt-cli on master*
› which rackt
/Users/christian/.nvm/versions/node/v6.0.0/bin/rackt
christian on dante in ~/git/rackt-cli on master*
› rackt test
The react/jsx-quotes rule is deprecated. Please use the jsx-quotes rule instead.
09 07 2016 15:57:18.387:WARN [plugin]: Error during loading "/Users/christian/.nvm/versions/node/v6.0.0/lib/node_modules/rackt-cli/node_modules/karma-phantomjs-launcher" plugin:
  Cannot find module 'phantomjs'
09 07 2016 15:57:18.556:WARN [karma]: No captured browser, open http://localhost:9876/
09 07 2016 15:57:18.562:INFO [karma]: Karma v0.13.19 server started at http://localhost:9876/
09 07 2016 15:57:18.573:INFO [launcher]: Starting browser Chrome
09 07 2016 15:57:19.502:INFO [Chrome 51.0.2704 (Mac OS X 10.11.5)]: Connected on socket /#-x-7A9HvnD5nTslFAAAA with id 27775402
Chrome 51.0.2704 (Mac OS X 10.11.5): Executed 0 of 0 ERROR (0.003 secs / 0 secs)

(that's running on the ract-cli repo, after fixing the lint errors)

@mzabriskie
Copy link
Owner

$ node -v && npm -v
v6.2.2
3.9.5

@CMTegner
Copy link
Contributor Author

CMTegner commented Jul 9, 2016

Have you npm link'd your global rackt-cli? This was causing some problems for me, but everything started working when I removed it and did a fresh npm i -g rackt-cli.

@mzabriskie
Copy link
Owner

Upon further investigation, it works when doing rackt test but fails when running npm test. There is something wrong when using the local node_modules/.bin/rackt compared to global.

@CMTegner CMTegner mentioned this pull request Jul 10, 2016
@CMTegner
Copy link
Contributor Author

Think I found the solution in #26.

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.

2 participants