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

remove ./node_modules/.bin from npm scripts #76

Closed
wants to merge 1 commit into from

Conversation

collinsauve
Copy link
Contributor

Fixes issue #74, which broke linting and tests on Windows.

This was referenced Mar 24, 2017
"prepublish": "./node_modules/.bin/babel src --out-dir lib",
"pretest": "./node_modules/.bin/babel src --out-dir lib",
"test": "npm run-script lint && ./node_modules/.bin/mocha --compilers js:babel-core/register"
"lint": "eslint src/**/*.js && eslint test/**/*.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work if eslint is not installed globally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to get this to work I normally have to modify my path to include :node_modules/.bin/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more in favor of the style of #75

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have eslint in my path, and I don't think I have it installed in npm global (npm ls -g | grep eslint is empty).

npm lint runs for me on windows (as do the other scripts).

@vhmth
Copy link
Collaborator

vhmth commented Mar 24, 2017

I like the dot-delimiter because it makes this repo self-contained. Right now this config makes the assumption that the user has exported node_modules from every subdir on their path. Does this not work with ./ as separate commands on Windows?

@paulius005
Copy link
Contributor

@vhmth looks like it does. Take a look @ #75

@vhmth
Copy link
Collaborator

vhmth commented Mar 24, 2017

My vote would be for ./ as separate commands for the Windows case.

@collinsauve
Copy link
Contributor Author

Here's a comment about this problem by isaacs (inventor of npm):

Also, never ever write ./node_modules/.bin/ in your scripts. Seriously, that should just throw. What if the dep is somewhere else, installed globally, or as a parent's dep? npm puts ./node_modules/.bin in the PATH environ for a reason. Do not tightly couple to non-portable platform-specific paths!

Note that he says "npm puts ./node_modules/.bin in the PATH". I did not need to install anything globally to get this to work.

@collinsauve
Copy link
Contributor Author

Does this not work with ./ as separate commands on Windows?

It works, but (as well as taking isaacs comment) I found it kind of hacky in that it creates scripts that really would only be used as building blocks for other scripts, and never run independently.

@vhmth
Copy link
Collaborator

vhmth commented Mar 24, 2017

https://www.useloom.com/share/e39a52d3b722475294409f092ef21e44

@collinsauve
Copy link
Contributor Author

:) Thanks for the insight. I don't know this kind of history / insight into npm - I wasn't aware it was this complex and debated.

I actually generally work in C# (just using JS for a chrome extension), so when you talk about "too much dynamic linking" going on in node - I definitely hear that!

So you've convinced me - since you value predictability and isolation over my arbitrary "cleanliness" of the scripts, #75 would be the way to go.

@vhmth
Copy link
Collaborator

vhmth commented Mar 24, 2017

@collinsauve cool glad I could convince you! There is no right or wrong answer here though. Totally game to leave it up to @tshaddix since he has to sign off on stuff like this too. :-)

@tshaddix
Copy link
Owner

tshaddix commented Jun 8, 2017

@vhmth I'm happy to go with whatever you see as the best option here :)

@tshaddix
Copy link
Owner

Closing this in favor of #83

@tshaddix tshaddix closed this Jun 14, 2017
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