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.

Sign up for GitHub

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

Jump to bottom

Use npm-run instead of shelljs-nodecli #3687

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Use npm-run instead of shelljs-nodecli #3687

merged 2 commits into from
Oct 18, 2016

Conversation

misteroneill
Copy link
Member

Description

Fixes #3683

Specific Changes proposed

Stop using shelljs-nodecli and start using npm-run! The reason this fails is that shelljs-nodecli makes the assumption that the package and its executable are the same name (e.g. the grunt command is exposed by the grunt package).

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Oct 14, 2016
@gkatsev
Copy link
Member

gkatsev commented Oct 14, 2016

I guess this is fine? I've never had problems with shelljs-nodecli before. And it's been working fine for me, so, not sure exactly what the problem was.

@misteroneill
Copy link
Member Author

The problem is that the package name and the CLI name don't match. shelljs-nodecli assumes they do; so, when you look for conventional-changelog it looks for node_modules/conventional-changelog/package.json and fails to find it and gives up.

@brandonocasey
Copy link
Contributor

is there any reason not to switch to vanilla shelljs and use shelljs.exec('node_modules/.bin/*')?

@misteroneill
Copy link
Member Author

I don't think that would work on Windows and I believe there are cases where node_modules/.bin isn't the right path? Hence the existence of the npm bin command.

@brandonocasey
Copy link
Contributor

LGTM

@misteroneill misteroneill added this to the 5.12 milestone Oct 17, 2016
@gkatsev
Copy link
Member

gkatsev commented Oct 17, 2016

fwiw, shelljs-nodecli is picking up the local conventional-changelog bin for me...

@gkatsev
Copy link
Member

gkatsev commented Oct 18, 2016

Or maybe not.

@BernardoFBBraga BernardoFBBraga mentioned this pull request Oct 18, 2016
Closed
gkatsev
gkatsev requested changes Oct 18, 2016
View reviewed changes
build/grunt.js
@@ -32,7 +32,7 @@ module.exports = function(grunt) {
release: {
tag_name: 'v'+ version.full,
name: version.full,
body: nodeCli.exec('conventional-changelog', '-p videojs', {silent: true}).output
body: npmRun.execSync('conventional-changelog -p videojs', {silent: true})
Copy link
Member

Choose a reason for hiding this comment

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

This needs an encoding: 'utf8' options because a buffer is returned by default.

@gkatsev gkatsev merged commit 8845bd3 into videojs:master Oct 18, 2016
@misteroneill misteroneill deleted the fix-build branch October 18, 2016 20:36
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkatsev gkatsev gkatsev approved these changes

Assignees
No one assigned
Labels
patch This PR can be added to a patch release.
Projects
None yet
Milestone
5.12
Development

Successfully merging this pull request may close these issues.

Loading "Gruntfile.js" tasks...ERROR
3 participants
@misteroneill @gkatsev @brandonocasey