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

npm test currently doesn't work without globally installing grunt-cli #1053

Closed
max-mapper opened this issue Oct 14, 2013 · 23 comments · Fixed by #2160, #2176 or #2218
Closed

npm test currently doesn't work without globally installing grunt-cli #1053

max-mapper opened this issue Oct 14, 2013 · 23 comments · Fixed by #2160, #2176 or #2218
Assignees
Milestone

Comments

@max-mapper
Copy link

npm test currently doesn't work in a freshly cloned + npm installed repo because grunt-cli isn't in devDependencies

commands in the scripts field of package.json e.g. {"scripts": {"start": "ghost"}} will resolve to the local module scope instead of requiring global installation. This means that working versions of modules can get bundled with applications.

for example, in the above example when npm start runs it will look for a module in the local available require scope that provides a ghost bin, and will use that. if it can't find a local module it will try the $PATH.

if I had two applications on my computer that both depend on ghost, but one depends on ghost v1 and another on ghost v2 (for example), it would suck if I had to npm install -g ghost@1.0.0 every time I went into the first project and then npm install -g ghost@2.0.0 in the other one. using npm scripts solves this problem. (I hope my explanation makes sense)

@jgable
Copy link
Contributor

jgable commented Oct 14, 2013

grunt-cli is not supposed to be in the devDependencies, it's a global module.

I think this is very clearly spelled out in the installation docs. Bullet point 4 to be exact.

@jgable
Copy link
Contributor

jgable commented Oct 14, 2013

More info about grunt and their architecture is available on their site.

@max-mapper
Copy link
Author

I should have mentioned the node community convention of npm install + npm start to get any project running, or npm install + npm test to run the test suite.

Requiring the installation of global modules (modules that provide bins can be used without global installation, as described above) adds potential headache to getting an app running, as users may have permission issues regarding global module installation. There's no such thing as a 'global-only' module.

It also means that if I ever distribute an app that uses ghost that I have to also force all of my users to install grunt-cli globally to get my app to run.

Closing this for now, just wanted to share how I've seen it done on NPM in the past.

@wolfeidau
Copy link

Having just tested this does enable running of tests from npm, although you still do require the sass and bourbon gems to be installed.

With these other external dependencies I do agree grunt-cli should be development dependency, mainly because it makes building in isolation a little easier, and more likely to succeed if grunt totally breaks compatibility again...

Just my 2 cents.

@jgable
Copy link
Contributor

jgable commented Oct 14, 2013

@maxogden Specifying grunt-cli as a development dependency would not solve the issue you are talking about. The reason grunt-cli must be installed globally is because it adds a bin command, grunt.

Have you tried to test that out? npm install grunt-cli locally and try to run grunt. Does it work for you? It doesn't for me. That is kind of what I meant by it being a global module.

Your larger issue about keeping a code base free of dependencies is nice in theory, but we like the power that some of these third party tools (like Grunt, Sass and Bourbon) offer our developers more than keeping the Node "purist" happy.

Thanks for the feedback though.

@wolfeidau Grunt separated the cli tool from the grunt framework for precisely the reason you mention, the cli tool is now just a harness for running your local project version of Grunt. Again, just take a look at the Grunt docs.

@max-mapper
Copy link
Author

@jgable again, there's never a scenario where global installation is required. here's how to use grunt without requiring it be installed globally:

npm install --save-dev grunt-cli

then add "start": "grunt" to package.json scripts

now run npm start

it currently throws an error because the Gruntfile.js in ghost is too greedy: https://github.com/TryGhost/Ghost/blob/master/Gruntfile.js#L40

@jgable
Copy link
Contributor

jgable commented Oct 14, 2013

That's interesting, I missed your thorough explanation in the first post of why it must be specified in the npm scripts for it to work.

The grunt command would not available any more which limits what we can do. For instance, we run grunt init to compile our assets on initial installation, what kind of script would that be? Another, we run grunt dev to compile assets, and start up a livereload server in development.

I found this info in the [npm-scripts] docs:

Additionally, arbitrary scripts can be run by doing npm run-script .

It would appear that you can define and run arbitrary scripts using this method. But, "init": "grunt init" in package.json, then npm run-script init does nothing for me.

If your argument is to make it a dependency just for npm test, that seems odd to me since we will definitely require it installed globally to get the site running which would cause the problems of conflicting versions anyway.

@max-mapper
Copy link
Author

weird, adding "init": "grunt init" to package.json and running npm run-script works for me (you can also use npm run init -- does the same thing)

i'm on node v0.10.18 and npm 1.3.8, could be that you're running an out of date node version

the so-called node 'purist' way to do it would be to have a npm script in your package.json for each thing that your package needs to execute, thereby not depending on global CLI commands. a major benefit of npm is encapsulation and allowing multiple versions of modules to get installed in the same dependency tree, and any required global dependencies open up the possibility of an app breaking at runtime because it got the wrong version of a dependency.

in the case of grunt it isn't so bad, as grunt-cli was written to address some of these problems, but it's nice to know how npm scripts at the very least for other situations in the future

@jgable
Copy link
Contributor

jgable commented Oct 15, 2013

Ok, I got npm run init working, my fault, had a typo. That seems useful. I'm not sure how @ErisDS feels about it though, I'm sure it would probably take a back seat for a while until things die down and we have an actual module published that people will npm install.

TIL all about npm scripts and local module scope.

@ErisDS
Copy link
Member

ErisDS commented Oct 15, 2013

@jgable thanks for taking point on this - I was soaking it all up.

I should have mentioned the node community convention of npm install + npm start to get any project running, or npm install + npm test to run the test suite.

If we can achieve this - I think that would be awesome. I think right now we're missing out on a way to do a git submodule init & update?

We'd also have to get smart in the grunt tasks about environments - running the right build script for the right environment. However, I think this is definitely the ideal to strive for, so I'm gonna reopen it.

@ErisDS ErisDS reopened this Oct 15, 2013
@jgable
Copy link
Contributor

jgable commented Oct 15, 2013

Here's what I think is going to be involved in this:

  • Add something to grunt init task that checks for and updates casper submodule if not present already.
  • Add scripts package.json for our major tasks (init, validate, prod, build)
  • Update all docs to use npm run <cmd> instead of grunt <cmd>
  • environments stuff?

@vladikoff
Copy link

As @maxogden pointed out the npm install && npm test pattern is popular and installing the global CLI to get started can be problematic for some users.

@shama created an issue gruntjs/grunt#943 regarding this. The goal is to make it possible for npm test to just run grunt test without relying on a global CLI

ErisDS added a commit that referenced this issue Nov 3, 2013
issue #1053

- not the greatest module, but perhaps one to re-create ourselves in future
jptacek pushed a commit to jptacek/Ghost that referenced this issue Nov 4, 2013
issue TryGhost#1053

- not the greatest module, but perhaps one to re-create ourselves in future
@kornelski
Copy link

FIY I've ran into this as well and raised issue in grunt-cli.

@shama
Copy link

shama commented Jan 9, 2014

An update: installing grunt-cli locally is now officially supported by Grunt: https://github.com/gruntjs/grunt-cli#installing-grunt-cli-locally

@gprasant
Copy link

I prefer running any tools like this from the local project hierarchy.

$ npm install --save-dev grunt-cli

$ ./node_modules/.bin/grunt will save us hours of potential head scratching in the future.

@hswolff hswolff mentioned this issue Feb 1, 2014
9 tasks
@ErisDS ErisDS modified the milestones: 0.5, Future Feb 8, 2014
@ErisDS
Copy link
Member

ErisDS commented Feb 10, 2014

Blah.. this isn't quite right on Windoze...

> git submodule update --init && bundle install && ./node_modules/.bin/grunt ini
t

Using sass (3.2.12)
Using thor (0.18.1)
Using bourbon (3.1.8)
Using bundler (1.3.5)
Your bundle is complete!
Use `bundle show [gemname]` to see where a bundled gem is installed.
'.' is not recognized as an internal or external command,
operable program or batch file.

npm ERR! ghost@0.4.1 install: `git submodule update --init && bundle install &&
./node_modules/.bin/grunt init`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the ghost@0.4.1 install script.
npm ERR! This is most likely a problem with the ghost package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     git submodule update --init && bundle install && ./node_modules/.bi
n/grunt init
npm ERR! You can get their info via:
npm ERR!     npm owner ls ghost
npm ERR! There is likely additional logging output above.
npm ERR! System Windows_NT 6.1.7601
npm ERR! command "c:\\Program Files\\nodejs\\node.exe" "c:\\Program Files\\nodej
s\\node_modules\\npm\\bin\\npm-cli.js" "install"
npm ERR! cwd c:\Ghost\Ghost
npm ERR! node -v v0.10.25
npm ERR! npm -v 1.3.24
npm ERR! code ELIFECYCLE
npm ERR!
npm ERR! Additional logging details can be found in:
npm ERR!     c:\Ghost\Ghost\npm-debug.log
npm ERR! not ok code 0

Weirdly, npm test works fine, but npm install only works if it is set to git submodule update --init && bundle install && ./node_modules/.bin/grunt init

I'm also not sure that git submodule update --init needs to be in here... grunt init will do this (although it is buggy and on my list of things to fix one day).

@ErisDS ErisDS reopened this Feb 10, 2014
@shama
Copy link

shama commented Feb 10, 2014

Try updating to git submodule update --init && bundle install && grunt init. npm will prefix the path ./node_modules/.bin in a platform safe way when running npm scripts. So grunt init within a npm script will default to the locally installed bin.

ErisDS added a commit to ErisDS/Ghost that referenced this issue Feb 11, 2014
fixes TryGhost#1053

- npm install doesn't need a path to grunt, and fails on Windows if it has one
- submodule update is handled by grunt (albeit badly... but let's fix that!)
@ErisDS ErisDS reopened this Feb 16, 2014
@ErisDS
Copy link
Member

ErisDS commented Feb 16, 2014

I'm not convinced we've got this working properly for everyone or that the documentation / contribution guidelines are correct given #2191

_travis.yml still has a line for installing global grunt-cli - is this still required?

@shama
Copy link

shama commented Feb 16, 2014

It works for me on OSX 10.9.1 and node.js v0.10.22, ruby-2.0.0-p247.

I did a fresh rm -rf Ghost && git clone git://github.com/TryGhost/Ghost && cd Ghost, npm i (which ran bundle install && grunt init from the npm install hook). Then npm start

I then removed my globally installed grunt-cli with npm uninstall -g grunt-cli and repeated the above.

- npm install -g grunt-cli in the before_install of the .travis.yml is no longer needed. Also in the contributing guidelines, 1. Run npm install -g grunt-cli is no longer needed. As well as grunt init as that will be ran automatically when the user does npm install.

The message: >> Local Npm module "grunt-cli" not found. Is it installed? comes from trying to load grunt-cli as a grunt task. Simply change the matchdep line in the Gruntfile.js to require('matchdep').filterDev(['grunt-*', '!grunt-cli']).forEach(grunt.loadNpmTasks); to avoid loading grunt-cli as a grunt task.

Happy to send a PR for any of the above if that is easier. Thanks!

@shama
Copy link

shama commented Feb 16, 2014

Another consideration, if Ghost is published to npm; the install script will be ran when end users do npm install ghost. Which will fail as grunt init requires grunt and all the other devDependencies.

I don't know what Ghost's plans are for publishing to npm but a proactive solution is to move the install script to another hook, such as:

"scripts": {
  "build": "bundle install && grunt init"
}

and ran with npm run build.

@ErisDS
Copy link
Member

ErisDS commented Feb 16, 2014

In short, I think we are trying to handle a few different use cases for Ghost, and potentially getting a bit confused as a result.

Case 1) Installing from npm, which runs npm install automatically.
Case 2) Cloning from git and running npm install

In the first case, the build is not necessary, as the npm package will contain built files the same as if you had downloaded a zip from Ghost.org or the GitHub releases page.

In the second case, the build is necessary, the same occurs if you run Ghost through Travis.

Our instructions, pretty much everywhere, tell people who are in case 1 - that is they have a pre-built ghost, to run npm install --production. They won't get the dev dependencies, and do not need bundle or to run grunt. Therefore I believe that @shama is correct - these scripts should be run as a 'build' script, and grunt-cli should stay in devDependencies.

I think the correct solution is to leave install blank, and to add

"scripts": {
     "build": "bundle install && grunt init && grunt prod",
     "pre-publish": "grunt && grunt prod"
}

As well as updating the Gruntfile to filter the grunt-cli task as mentioned in @shama's first comment:

require('matchdep').filterDev(['grunt-*', '!grunt-cli']).forEach(grunt.loadNpmTasks);

This was referenced Feb 16, 2014
@jgable
Copy link
Contributor

jgable commented Feb 17, 2014

👍

@hswolff
Copy link
Contributor

hswolff commented Feb 17, 2014

Case 1) Installing from npm, which runs npm install automatically.

As you noted @ErisDS there is no need to run any npm script in this scenario as the built scripts will be included in the npm package.

Adding "prepublish": "grunt && grunt prod" to the package.json file will rightly ensure the built files are always present.

Case 2) Cloning from git and running npm install

Small nit, but I'd argue we should move development specific commands back to the Gruntfile.js as that is where all development tasks currently reside. Adding them to the package.json file adds another location that will require maintenance, which isn't necessary.

hswolff added a commit to hswolff/Ghost that referenced this issue Feb 19, 2014
expected behavior of npm packages

fixes TryGhost#1053

- updates travis config to be more in line with
 current dev steps

- fix `grunt-cli` warning from appearing when
 running grunt
@ErisDS ErisDS modified the milestones: 0.4.2, 0.5 Mar 10, 2014
kevinansfield added a commit that referenced this issue Nov 9, 2023
refs #18752, TryGhost/Product#3897, TryGhost/Product#4112, TryGhost/Product#4104, #18866, #18753, TryGhost/Product#4116, #18888, #18844

- 🐛 Fixed browser focus on editor when clicking card ([Koenig/#1051](TryGhost/Koenig#1051))
- 🐛 Fixed signup card styles with image background ([Koenig/#1052](TryGhost/Koenig#1052))
- 🐛 Fixed slash menu having fixed position when scrolling ([Koenig/#1054](TryGhost/Koenig#1054))
- 🐛 Fixed signup card text color with transparent background ([Koenig/#1053](TryGhost/Koenig#1053))
- 🐛 Fixed text formats being lost when copy/pasting from Google Docs ([Koenig/#1055](TryGhost/Koenig#1055))
- 🐛 Fixed pasting link behaviour in single line nested editors ([Koenig/#1056](TryGhost/Koenig#1056))
- 🐛 Fixed backspace behaviour at start of aside/quote ([Koenig/#1057](TryGhost/Koenig#1057))
- 🐛 Fixed text having unexpected formats when rendering ([Koenig/#1058](TryGhost/Koenig#1058))
- 🐛 Fixed placeholder descenders being cut off in nested editor ([Koenig/#1059](TryGhost/Koenig#1059))
- 🐛 Fixed HTML->Lexical conversion not handling paragraphs inside blockquotes ([Koenig/#1061](TryGhost/Koenig#1061))
allouis pushed a commit that referenced this issue Nov 15, 2023
refs #18752, TryGhost/Product#3897, TryGhost/Product#4112, TryGhost/Product#4104, #18866, #18753, TryGhost/Product#4116, #18888, #18844

- 🐛 Fixed browser focus on editor when clicking card ([Koenig/#1051](TryGhost/Koenig#1051))
- 🐛 Fixed signup card styles with image background ([Koenig/#1052](TryGhost/Koenig#1052))
- 🐛 Fixed slash menu having fixed position when scrolling ([Koenig/#1054](TryGhost/Koenig#1054))
- 🐛 Fixed signup card text color with transparent background ([Koenig/#1053](TryGhost/Koenig#1053))
- 🐛 Fixed text formats being lost when copy/pasting from Google Docs ([Koenig/#1055](TryGhost/Koenig#1055))
- 🐛 Fixed pasting link behaviour in single line nested editors ([Koenig/#1056](TryGhost/Koenig#1056))
- 🐛 Fixed backspace behaviour at start of aside/quote ([Koenig/#1057](TryGhost/Koenig#1057))
- 🐛 Fixed text having unexpected formats when rendering ([Koenig/#1058](TryGhost/Koenig#1058))
- 🐛 Fixed placeholder descenders being cut off in nested editor ([Koenig/#1059](TryGhost/Koenig#1059))
- 🐛 Fixed HTML->Lexical conversion not handling paragraphs inside blockquotes ([Koenig/#1061](TryGhost/Koenig#1061))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment