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

dep: add grunt-cli #2830

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@MylesBorins
Contributor

MylesBorins commented Dec 22, 2015

currently the project relies on grunt-cli to run unit tests.
There are no instructions in the readme to inform developers of this dependency.

This PR adds grunt-cli to the dev-dependencies for a more seamless development
process. This PR will also make moment compatible with CITGM
the node.js smoketesting utility. This will allow moment's unit test suite to
be ran against every release of Node.js, giving us an opportunity to give you a heads
up if anything breaks before a release.

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Dec 22, 2015

It would appear that installing the dependency with --save-dev has re organized much of the package.json and created quite a bit more churn then necessary. If this is undesirable I'll force push an update (no need to break current CI run)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 28, 2015

According to this you should install grunt-cli yourself, and I agree we can update the docs to mention that. Putting grunt-cli in the project.json file is only helpful if you plan to run everything with node_modules/.bin/grunt instead of grunt, which is not suggested.

@icambron

This comment has been minimized.

Member

icambron commented Dec 30, 2015

The docs do say that. From Contributing:

git clone https://github.com/moment/moment.git
cd moment
npm install -g grunt-cli
npm install
git checkout develop  # all patches against develop branch, please!
grunt                 # this runs tests and jshint

@icambron icambron closed this Dec 30, 2015

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

@ichernev @icambron while I realize this is the promoted best practice from grunt, it will make it that we over on the node project cannot use moment in our smoke testing suite.

That's a shame.

I'm going to pop over to grunt-cli and see if we can get them to update their suggestion

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

@ichernev @icambron I would check out gruntjs/grunt-cli#90 (comment)

@vladikoff states that grunt-cli will likely update their docs as the new standard is indeed including grunt-cli in the package.json

@icambron

This comment has been minimized.

Member

icambron commented Jan 4, 2016

It's fine with me, then.

@icambron icambron reopened this Jan 4, 2016

@vladikoff

This comment has been minimized.

vladikoff commented Jan 4, 2016

👍

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

🎉🎉🎉🎉🎉🎉

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

I'll update the docs in contributing as well

dep: add grunt-cli
currently the project relies on grunt-cli to run unit tests.
There are no instructions in the readme to inform developers of this dependency.

This PR adds grunt-cli to the dev-dependencies for a more seamless development
process. This PR will also make moment compatible with [CITGM](https://github.com/nodejs/citgm)
the node.js smoketesting utility. This will allow moment's unit test suite to
be ran against every release of Node.js, giving us an opportunity to give you a heads
up if anything breaks before a release.
@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

So I modified the change to not cause as much churn. The one thing I am noticing is that currently the workflow suggests calling to grunt directly

grunt will not exist in the path of the system if installed locally, it would only be accessible via an npm script.

This may not make a ton of sense to do if there are not npm aliases for each of the grunt tasks, which in a way seems like it might defeat some of ease of using grunt in the first place

@icambron

This comment has been minimized.

Member

icambron commented Jan 4, 2016

After thinking about it, I think we should leave the instructions as is. Most people will just want to call grunt and the easiest way to get that is to install it globally. I don't think it really hurts anything to be installed in both places. If someone figures out an easy way to call locally installed NPM executables, we can change the instructions.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jan 4, 2016

Can you explain why can you run your tests? Just install grunt-cli globally or locally and use node_modules/.bin/grunt to run it. What is preventing you from doing either of those? Why do you want to run grunt tests on moment...we run them on every release :)

@MylesBorins

This comment has been minimized.

Contributor

MylesBorins commented Jan 4, 2016

@ichernev this is part of a batch job that is running the unit tests of the most depended upon packages in the node ecosystem to be run in Continuous Integration on changes done to node itself.

We would like to avoid having to bootstrap the testing environment, specifically to keep tests as self contained as possible.

I'm going to have a mull over this and try to find a reasonable solution. Adding grunt-cli fixes our edge case, but makes little sense to do in your project if people need to globally link it to follow your dev workflow.

Not really sure what the best solution is here.

Feel free to close this PR and I'll come back with findings if I have found a reasonable way to approach this.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jan 9, 2016

Merged in 542bacd

@ichernev ichernev closed this Jan 9, 2016

ichernev added a commit that referenced this pull request Jan 9, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jan 9, 2016

If this helps you develop node/io itself, I'm merging it.

I guess if you use npm test, it first installs stuff, and then grunt test will fetch grunt from node_modules/.bin, so it would work.

@mj1856 mj1856 added this to the 2.11.1 milestone Jan 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment