Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(*): remove usage of global grunt-cli #16276

Merged
merged 1 commit into from
Nov 11, 2017
Merged

docs(*): remove usage of global grunt-cli #16276

merged 1 commit into from
Nov 11, 2017

Conversation

frederikprijck
Copy link
Contributor

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Docs update

What is the current behavior? (You can also link to an open issue here)
Currently, the contributing docs tell people to globally install grunt and build angularjs using the grunt cli command.

What is the new behavior (if this is a feature change)?
This commit ensures grunt is not required globally by updating all the commands to use yarn grunt instead of grunt.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

@frederikprijck
Copy link
Contributor Author

Personally, I prefer to use scripts. This would result in commands such as yarn package instead of yarn grunt package. This can still be modified in the current PR if wanted, but I'd like to discuss it prior to changing it.

@Narretz
Copy link
Contributor

Narretz commented Oct 14, 2017

How about we add a note that you don't have to install grunt-cli globally if you prefix the grunt commands with yarn?

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Oct 14, 2017

@Narretz Do you mean to undo the changes of this commit and only add the note ?
What's the point of using the global grunt cli by default? Using global modules for these kind of things doesn't have any benefits, does it?

@petebacondarwin
Copy link
Contributor

My reading of this PR is that it is not using a global grunt install anymore.

@Narretz
Copy link
Contributor

Narretz commented Oct 16, 2017

Using global modules for these kind of things doesn't have any benefits, does it?

It does have the benefit that you don't have to prefix each command with yarn . That's 5 keystrokes.

Do you mean to undo the changes of this commit and only add the note?

That's right. Another problem is that this is not the only place where these instructions are. There's also https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md, https://github.com/angular/angular.js/blob/master/README.md, even on the angularjs.org repo: https://github.com/angular/angularjs.org

@gkalpak
Copy link
Member

gkalpak commented Oct 16, 2017

Personally, I prefer to use scripts. This would result in commands such as yarn package instead of yarn grunt package.

For better or worse, we are using grunt as our task runner. At this point, I don't see much benefit in implementing "aliases" for all grunt commands (and having to keep the yarn scripts up-to-date with the grunt commands in case we add/remove any).

How about we add a note that you don't have to install grunt-cli globally if you prefix the grunt commands with yarn?

I would go the other way and document that you can drop the yarn part in yarn grunt ... if you install grunt globally (but keep the uing the local version "by default").

👍 for updating all places which refer to grunt commands 😃

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Oct 16, 2017

At this point, I don't see much benefit in implementing "aliases" for all grunt commands

Ye makes sense.

Using scripts does hide the actual implementation from the usage of the commands (whether you're using grunt, gulp, webpack, pure node, ... you can still use the same cli command).

As AngularJS is using both Grunt and Gulp (atleast, they are both installed), one could run grunt and gulp without the user/developer needs to know which one he is running by using scripts. Currently, I have no idea what Gulp is being used for (afaik I shouldn't need to know it and just rely on the scripts).

But yes, the downside is you need to maintain the scripts.

@Narretz
Copy link
Contributor

Narretz commented Oct 16, 2017

If we document yarn grunt ... as default, with the option to install grunt-cli globally, then that's fine with me

@Narretz
Copy link
Contributor

Narretz commented Nov 3, 2017

This PR must / can be updated now. There's now things to change in the DEVELOPERS.md, too.

@Narretz Narretz added this to the 1.7.0 milestone Nov 3, 2017
@frederikprijck
Copy link
Contributor Author

frederikprijck commented Nov 8, 2017

Alright, I've moved the changes to the new files. I didn't add the message regarding global gunt yet, will do soon.

I was wondering how the following ended up in there and I wanted to check if this is really necessary ?

Note: If you're using Linux, and yarn install fails with the message 'Please try running this command again as root/Administrator.', you may need to globally install grunt and bower:

I've had this problem on my mac for a while (prior to my npm permissions where set correctly).
Imho this should not result in global installation of the npm packages, instead there could be two solutions:

  • sudo the local commands (but we don't want that)
  • fix permission issues.

Or am I missing something why the global installation is really required? Is it realy up to us to define how they have to fix their permission issues ? We might be good with linking to https://docs.npmjs.com/getting-started/fixing-npm-permissions, no ?

@Narretz
Copy link
Contributor

Narretz commented Nov 8, 2017

I always go to the git blame to find why something was added. Leads to result in 50% of the cases. ;)
This was originally added for npm and then simply changed to yarn. 1dedcdf#diff-acca20ff7c21d5e56c51120c19837a7fL81 I'm okay with adding a link to yarn docs for permission issues.

DEVELOPERS.md Outdated
sudo yarn global add grunt-cli
sudo yarn global add bower
```
**Note:** If you're using Linux, and `yarn install` fails with the message 'Please try running this command again as root/Administrator.', you may need to fix your [npm permissions](https://docs.npmjs.com/getting-started/fixing-npm-permissions).
Copy link
Contributor

Choose a reason for hiding this comment

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

Why npm when we are using yarn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, it's a common issue that permissions are set wrong on osx and Linux for npm, but I'll check how that works for yarn!

Copy link
Contributor Author

@frederikprijck frederikprijck Nov 9, 2017

Choose a reason for hiding this comment

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

@Narretz I had a chat offline with @gkalpak and we think it's probably a good idea to just leave this part out and reconsider if there are issues with this. What do you think?

Encouraging sudo is definetly not what we want to be doing, and I'm not sure we want to maintain npm/yarn permission issues in the angularjs repo either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine with me

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Nov 10, 2017

I've added the mentioning of installing grunt globally:

Grunt: We use Grunt as our build system. We're using it as a local dependencies but if you prefer to use the command-line tool globally, you can install it using yarn:

yarn global add grunt-cli

I'm still a bit unsure what the benefit is for adding this to the docs, as it realy is not recommended (it's even recommended to avoid it) to install it globally. If it's not recommended, why guide anyone to adding it globally ?

@Narretz
Copy link
Contributor

Narretz commented Nov 10, 2017

Because then you can grunt ... instead of yarn grunt ... At the moment, the message isn't very helpful if you don't mentiond that you can skip the yarn part in that case. And the Grunt docs still advise that you install the cli globally.
The yarn docs also say there are use cases for global adding: https://yarnpkg.com/lang/en/docs/cli/global/

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Nov 10, 2017

Let me add that part aswell.

Because then you can grunt ... instead of yarn grunt ...

Even tho thats true, the issues global NPM modules can cause are worse.
Imagine contributing to several projects, each using a seperate, globally, grunt version ... This is pretty much something I had issues with when I tried setting up angularjs for the first time due to the global grunt, iirc I had to remove my other global installed grunt (which was my own mistake, as I should not have had grunt globally in the first place).

I do agree that if that's not the case, installing it globally results in "shorter" commands. But promoting the global installation is odd as it's realy something to be avoided at all cost.

@frederikprijck
Copy link
Contributor Author

frederikprijck commented Nov 10, 2017

And the Grunt docs still advise that you install the cli globally.

Sadly, alot of libraries mention it coz it's easier to explain how the CLI works without explaining how NPM/Yarn resolves dependencies (localy vs global) and how scripts work (which is out of scope of the grunt docs anyway).

The yarn docs also say there are use cases for global adding

Ofcourse there are. Every none-project dependency should be globally (yarn, yeoman, angular/cli, ...).
Every project dependency should be in the project in such a way that the project is as self-contained as possible, resulting in less hassle to setup a project.

Anyway, I've added the message 😄

DEVELOPERS.md Outdated
@@ -34,13 +34,15 @@ machine:
to be installed and included in your
[PATH](http://docs.oracle.com/javase/tutorial/essential/environment/paths.html) variable.

* [Grunt](http://gruntjs.com): We use Grunt as our build system. Install the grunt command-line tool
globally with:
* [Grunt](http://gruntjs.com): We use Grunt as our build system. We're using it as a local dependencies but if you prefer to use the command-line tool
Copy link
Contributor

@Narretz Narretz Nov 10, 2017

Choose a reason for hiding this comment

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

Sorry to bother you so long with this :D but I feel like we can be more concise and precise here. How about

We're using it as a local dependency, but you can also add the grunt command-line tool globally (with yarn global add grunt-cli), which allows you to leave out the yarn prefix for all our grunt commands.

Then you can remove the code block and note below (which makes it sound like you can only use global or local grunt, but you can still use yarn grunt ... if you want, even with global grunt-cli).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to feel sorry, I appreciate the feedback! I do think it reads better the way u propose, let me update it!

Copy link
Contributor

Choose a reason for hiding this comment

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

I will merge when you've updated this part 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 😄

Previously, the `DEVELOPERS.md` and `CONTRIBUTING.md` files
refered to global `grunt-cli` by default.

This commit ensures the local `grunt-cli` is used by default
and mentiones the possibility to still use the global `grunt-cli`.
@frederikprijck frederikprijck changed the title docs(misc): Updating contribution docs to avoid installation of grunt… docs(*): remove usage of global grunt-cli Nov 10, 2017
@Narretz Narretz merged commit f876ab7 into angular:master Nov 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants