Remove ShellJS copy and use the NPM version #5912
Conversation
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| 'use strict'; | |||
|
|
|||
| require('./external/shelljs/make'); | |||
| require('shelljs/make'); | |||
There was a problem hiding this comment.
I'm wondering if it would be a good idea to enclose this in a try... catch block, e.g. something like this:
try {
require('shelljs/make');
} catch (ex) {
console.log('Run "npm install"'); // Probably needs a better message
return;
}When I tested the patch the first time, running any of the make commands just threw an error, and it took me a couple of seconds to realise that I needed to run npm install.
For less experienced users, it's not unlikely that they could get stuck on this, so a message telling them what to do seems like a good idea to me.
Edit: A somewhat fun fact is that if the above is implemented, I think that it would actually make PR #5908 obsolete :-)
There was a problem hiding this comment.
Done. Great catch, thanks for noticing this!
There is no need to have a copy of ShellJS in the repository as it is also available on NPM. The NPM version is also much newer. This way we do not have to update this anymore and let NPM do that automatically.
The previous commit implements a check for ShellJS and otherwise prompts the user to run "npm install". New clones of the codebase will need "npm install" for ShellJS and therefore automatically install jshint. Existing clones of the codebase will also need "npm install" again since ShellJS needs to be installed using NPM as it is not in the "external" folder anymore. Since everyone will get this prompt and install everything automatically, we will never reach this code path anymore. This patch makes mozilla#5908 obsolete and reduces code complexity for the lint target. Thanks to @Snuffleupagus for noticing this!
|
/botio preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/2f5c55fff8ff874/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/fc11619ae44cdb7/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/2f5c55fff8ff874/output.txt Total script time: 0.82 mins Published |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/fc11619ae44cdb7/output.txt Total script time: 1.04 mins Published |
|
This does look perfectly fine to me, and it'd be great to be able to get rid of all this code from the repo! /botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c20f1463f1e9d36/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/dcb77ac0c4192fe/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/dcb77ac0c4192fe/output.txt Total script time: 2.97 mins
Image differences available at: http://107.22.172.223:8877/dcb77ac0c4192fe/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/c20f1463f1e9d36/output.txt Total script time: 22.98 mins
|
Remove ShellJS copy and use the NPM version
|
Thank you! |
|
@timvandermeij we probably need to fix version number to be exact to avoid changes similar to shelljs/shelljs#207 |
There is no need to have a copy of ShellJS in the repository as it is also available on NPM. The NPM version is also much newer. This way we do not have to update this anymore and let NPM do that automatically.