refactor npm install (again)

[Narretz]

No description provided.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[petebacondarwin]

@IgorMinar - can you take a quick look at this when you get a chance?

[mgol]

Why this else? rimraf is cross-platform.

[Narretz]

I think rm -rf might still be more robust, so we should use it if possible. @IgorMinar should probably have a word here.

[petebacondarwin]

@IgorMinar wanted to use rm -rf where possible for performance reasons

[Narretz]

This has been confirmed to work on Windows in #11143 so I'm gonna merge it at the end of the week unless somebody has any objections.

[Narretz]

Although I'd like @IgorMinar to take a quick look. :)

[petebacondarwin]

Let's go with this @Narretz

[Narretz]

Aah, travis was actually using the install-dependencies.sh script. We need to fix this first.

[Narretz]

@petebacondarwin do you have an idea how we could handle this? I fear we have to duplicate the functionality, because we can't use the js file when travis is installing.

[petebacondarwin]

Hmm. That is a pain. And of course this doesn't work if npm install has not already been run as rimraf would not be available anyway.
Perhaps we just need an option not to use this at all on Windows?

[mgol]

I fear we have to duplicate the functionality, because we can't use the js file when travis is installing.

Why can't we? Node is already available, instead of:

  - scripts/npm/install-dependencies.sh

we'd just use:

  - node scripts/npm/install-dependencies.js

What am I not seeing here?

[petebacondarwin]

@mzgol the problem is that the new version of the install-dependencies.xxx, which happens to be a Grunt task now, relies upon things like:

var rimraf = require('rimraf');

which of course will fail if we have not run npm install at least once already as that module will not yet exist locally.

Or am I missing something?

[mgol]

@petebacondarwin Right... Could we just include rimraf manually in the repository and require it from this specific folder instead of node_modules? It's just 412 KB.

Or we could copy over the rimraf.js file and edit out glob which adds most of these 412 KB since we don't use glob patterns in our command (it would make it harder to upgrade the package later, though).

EDIT: We could also copy rimraf.js and instead of editing glob out of it, define a custom glob object that will just throw if used. Since we don't use this code path it would work and it would be a little easier to update rimraf in the future.

Not sure if that's worth the complexity, though. But it would decrease 412 KB to 7.3 KB.

[petebacondarwin]

On non-Windows machines we can just use rm -rf. It is only on Windows machines that we need an alternative solution. So I think that we should try to do the switch at a higher level.

[petebacondarwin]

Closing in favour of #12792