GitHub user stevengill opened a pull request:

https://github.com/apache/cordova-lib/pull/407

CB-9858 Cordova Fetch Work

This is not ready to merge yet! Opening the PR so it is easy to manage diffs and take feedback

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stevengill/cordova-lib CB-9858

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-lib/pull/407.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #407

commit 378b876bdc7aa4e80450e3a3e3d45f02891fc50d
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-02-06T02:13:51Z

initial commit for cordova-fetch

commit 2206b1e2d8976dd2d0b21adf7ee4042b5eeec462
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-02-09T02:24:41Z

implemented fetch module and added tests

commit 1e3853d0e768dd9a7af63e026643601ac900a5d6
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-02-24T02:03:40Z

CB-9858: completed first implementation of cordova-fetch module

commit de15f74e7872fc249346faf152e63bb061036a5f
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-02T02:11:08Z

CB-9858 updated tests

commit 9a1373571b082f1a8e6e643306f89c204ea9fcd5
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-04T01:31:49Z

CB-9858 adding platforms can now use cordova-fetch with --fetch flag

commit 47ae946562058fb7563685d1fdee61d7b7fcbf9f
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-09T01:10:17Z

CB-9859 added trimID to handle cordova platform update usecase

commit 1fa38af3c3b6a35cb84d7e01b651b5aafef33e28
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-09T01:30:22Z

CB-9858 cordova platform remove --fetch deletes module from node_modules directory

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-194059148

I decided to write the tests in node-tap. I am now regretting that decision a bit. Tests randomly timeout and fail. Doesn't happen in actual use.

GitHub user stevengill opened a pull request:

https://github.com/apache/cordova-cli/pull/239

CB-9858 added --fetch option

Needs https://github.com/apache/cordova-lib/pull/407

Part of https://github.com/cordova/cordova-discuss/pull/33

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stevengill/cordova-cli CB-9858

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-cli/pull/239.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #239

commit ac24fde7dba444aabc143ff04fe9f5d7ed8872e4
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-04T00:32:37Z

CB-9858 added --fetch option

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-194065776

Needs https://github.com/apache/cordova-cli/pull/239.

Discussion at https://github.com/cordova/cordova-discuss/pull/33

Github user nikhilkh commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55472886

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(!shell.which('npm')) {
— End diff –

Judging from previous experience with our tools not being able to find ANDROID_HOME/JAVA_HOME - this might become an error message that will affect new people. We should consider doing a best effort to find the npm installed on the system if `which` can't find it. There are some well known places on Windows/Mac where this gets installed.

Github user nikhilkh commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55473246

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(!shell.which('npm'))

+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
— End diff –

Are the `opts` for superspawn the same as the `opts` passed to this function?

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55480086

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(!shell.which('npm')) {
— End diff –

Sounds like a good idea! Do you have any examples of doing this with Java_home I could use as a basis for this? I pretty much mickimed how our current fetch code looks for git.

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55480177

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(!shell.which('npm'))

+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
— End diff –

Yup! Pass everything to superspawn that fetch was passed. Add the cwd to opts though.

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-194441672

To-do: add cordova-lib tests for this

Github user nikhilkh commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55567900

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(!shell.which('npm')) {
— End diff –

https://github.com/apache/cordova-android/blob/master/bin/lib/check_reqs.js#L97. We end up modifying process.env["PATH"] with probable directories for JAVA_HOME/ANDROID_HOME before invoking superspawn.

Github user nikhilkh commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55575046

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
— End diff –

I was trying to give it a spin but I get this error:
λ cordova platform add android --verbose
Error: Cannot find module 'q'
at Function.Module._resolveFilename (module.js:336:15)
at Function.Module._load (module.js:286:25)
at Module.require (module.js:365:17)
at require (module.js:384:17)
at Object.<anonymous> (D:\cordova\cordova-lib\cordova-fetch\index.js:20:9)

Looks like the package.json for this module needs more dependencies.

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r55578321

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,176 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
— End diff –

You are correct. Looks like I had it locally but didn't save it. I'll update this when I land and get to my computer.

GitHub user stevengill opened a pull request:

https://github.com/apache/cordova-plugman/pull/87

CB-9858 added --fetch option to plugman

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stevengill/cordova-plugman CB-9858

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cordova-plugman/pull/87.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #87

commit 5af3180aa03c1f37ef07d343ee9d5385d1a4248d
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-03-22T06:41:27Z

CB-9858 added --fetch option to plugman

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-199661470

Plugman PR adding --fetch flag support https://github.com/apache/cordova-plugman/pull/87

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r60898584

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,226 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
+
+ //if user added --save flag, pass it to npm install command
+ if(opts.save)

+
+ //check if npm is installed
+ isNpmInstalled();
— End diff –

Probably it makes sense to check this at the very beginning of the method

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r60899092

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,226 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
+
+ //if user added --save flag, pass it to npm install command
+ if(opts.save)

+
+ //check if npm is installed
+ isNpmInstalled();
+
+ //Grab json object of installed modules before npm install
+ return depls(dest)
+ .then(function(depTree) {
+ tree1 = depTree;
+
+ //install new module
+ return superspawn.spawn('npm', fetchArgs, opts);
— End diff –

Perhaps it is worth to add `--production` flag to avoid installing `devDependencies`

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r60995226

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,226 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
+
+ //if user added --save flag, pass it to npm install command
+ if(opts.save)

+
+ //check if npm is installed
+ isNpmInstalled();
— End diff –

True. I'll move it up

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r60995427

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,226 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A module that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ if(dest && target) {
+ //add target to fetchArgs Array
+ fetchArgs.push(target);
+
+ //append node_modules to dest if it doesn't come included
+ if (path.basename(dest) !== 'node_modules')

+
+ //create dest if it doesn't exist
+ if(!fs.existsSync(dest))

+
+ } else return Q.reject(new CordovaError('Need to supply a target and destination'));
+
+ //set the directory where npm install will be run
+ opts.cwd = dest;
+
+ //if user added --save flag, pass it to npm install command
+ if(opts.save)

+
+ //check if npm is installed
+ isNpmInstalled();
+
+ //Grab json object of installed modules before npm install
+ return depls(dest)
+ .then(function(depTree) {
+ tree1 = depTree;
+
+ //install new module
+ return superspawn.spawn('npm', fetchArgs, opts);
— End diff –

I don't think it is necessary. It is running `npm install` for modules from registry or git url. `devDependencies` shouldn't be getting installed in this scenario regardless of `--production`

Github user codecov-io commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-215918679

1. 1. [Current coverage][cc-pull] is *13.19%*
      > Merging 407[cc-pull] into [master][cc-base-branch] will increase coverage by *-86.81%*

```diff
@@ master #407 diff @@
==========================================
Files 69 34 -35
Lines 9878 3261 -6617
Methods 841 525 -316
Messages 0 0
Branches 1271 636 -635
==========================================

• Hits 9878 430 -9448
• Misses 0 2831 +2831
  Partials 0 0
  ```

1. 3 files (not in diff) in `cordova-lib/src/util` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F7574696C)

• Misses `+46`
• Hits `-86`
  1. 4 files (not in diff) in `...dova-lib/src/plugman` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E)
• Misses `+219`
• Hits `-437`
  1. 2 files (not in diff) in `...va-lib/src/platforms` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F706C6174666F726D73)
• Misses `+234`
• Hits `-444`
  1. 3 files (not in diff) in `...ordova-lib/src/hooks` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F686F6F6B73)
• Misses `+159`
• Hits `-315`
  1. 2 files (not in diff) in `...etadata/parserhelper` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F76612F6D657461646174612F70617273657268656C706572)
• Misses `+24`
• Hits `-84`
  1. 9 files (not in diff) in `...src/cordova/metadata` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F76612F6D65746164617461)
• Misses `+859`
• Hits `-1578`
  1. 7 files (not in diff) in `...dova-lib/src/cordova` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F7661)
• Misses `+440`
• Hits `-855`
  1. 1 files (not in diff) in `cordova-lib/src` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F737263)
• Misses `+22`
• Hits `-42`
  1. 2 files in `...dova-lib/src/cordova` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F7372632F636F72646F7661)
• Misses `+716`
• Hits `-1327`
  1. 1 files in `cordova-lib/src` were modified. [more](https://codecov.io/gh/apache/cordova-lib/commit/b61258fd4d50e6f1b71b55a6200a4e73b7d1857f/changes?src=pr#636F72646F76612D6C69622F737263)
• Misses `+73`
• Hits `-136`

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [5867657...b61258f][cc-compare]
[cc-base-branch]: https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr
[cc-compare]: https://codecov.io/gh/apache/cordova-lib/compare/5867657d989b136702fe2f14172661ca489573ed...b61258fd4d50e6f1b71b55a6200a4e73b7d1857f
[cc-pull]: https://codecov.io/gh/apache/cordova-lib/pull/407?src=pr

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-216691951

Hey Everyone,

I think the first implementation of this is ready to hit master behind the --fetch flag. It includes platform and plugin fetching + removing.

Take a look @nikhilkh @purplecabbage @vladimir-kotikov

Needs: apache/cordova-cli#239 and apache/cordova-plugman#87

I will add template fetching next.

I did run into some issues with our cordova tests. The tests finish before the promise resolves. Probably a issue with how we are handling promises in cordova, but could also be related to us using jasmine 1.3 instead of jasmine 2.x. I'll continue to investigate but not a blocker. My manually testing shows that the uninstall command works fine with cordova.

cordova-fetch tests are written using jasmine 2.0. No issues there.

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r61998292

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
— End diff –

Nit: the JSDoc notation for multiple types is a bit different - single `|` instead `||` (please see http://usejsdoc.org/tags-type.html).

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r61998473

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

obj1 json object representing installed modules before latest npm install
+ * @param

obj2 json object representing installed modules after latest npm install
+ *
+ * @return

target target that was passed into cordova-fetch.
+ * can be moduleID, moduleID@version or gitURL
+ *
+ * @return

id the packageID
+ * @param

dest destination of where to fetch the modules
+ *
+ * @return

Returns the absolute url for the module or throws a error
— End diff –

Same here

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r61999817

— Diff: cordova-fetch/spec/package.json —
@@ -0,0 +1,11 @@
+{
— End diff –

Do we need a separate `package.json` for specs?

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62000572

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ //check if npm is installed
+ isNpmInstalled();
— End diff –

Looks like in case if `npm` is not available this will return a promise rejection which is never propagated to the caller. Probably you need to wrap this call and all the code below into top-level promise (similar to #384). This would also protect the code against CB-10519(https://issues.apache.org/jira/browse/CB-10519)

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62000656

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

obj1 json object representing installed modules before latest npm install
+ * @param

opts [opts=

] options to pass to npm uninstall
+ *
+ * @return

Returns a promise with the npm uninstall output or an error.
+ *
+ */
+module.exports.uninstall = function(target, dest, opts) {
+ var fetchArgs = ['uninstall'];
+ opts = opts || {};
+
+ //check if npm is installed on the system
+ isNpmInstalled();
— End diff –

The same issue with not rejecting the promise in case of missing `npm`

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62100600

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
— End diff –

Thanks! I'll fix that

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62100822

— Diff: cordova-fetch/index.js —
@@ -0,0 +1,234 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ 'License'); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+var Q = require('q');
+var shell = require('shelljs');
+var superspawn = require('cordova-common').superspawn;
+var events = require('cordova-common').events;
+var depls = require('dependency-ls');
+var path = require('path');
+var fs = require('fs');
+var CordovaError = require('cordova-common').CordovaError;
+var isUrl = require('is-url');
+
+/*
+ * A function that npm installs a module from npm or a git url
+ *
+ * @param

dest destination of where to install the module
+ * @param

opts [opts=

] options to pass to fetch module
+ *
+ * @return

Returns string of the absolute path to the installed module.
+ *
+ */
+module.exports = function(target, dest, opts) {
+ var fetchArgs = ['install'];
+ opts = opts || {};
+ var tree1;
+
+ //check if npm is installed
+ isNpmInstalled();
— End diff –

Your right. I originally has it connected to the promise change but removed it during a refactor. I'll fix this!

Github user stevengill commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62101897

— Diff: cordova-fetch/spec/package.json —
@@ -0,0 +1,11 @@
+{
— End diff –

I just copy it into temporary tests folder so I can test - - save. Any alternative suggestion? Should I store it somewhere else?

Github user vladimir-kotikov commented on a diff in the pull request:

https://github.com/apache/cordova-lib/pull/407#discussion_r62104877

— Diff: cordova-fetch/spec/package.json —
@@ -0,0 +1,11 @@
+{
— End diff –

Hmm. Maybe just name it differently. My concern is that presence of another `package.json` might be a bit confusing + break some tools in some scenarios

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-217087853

Made the changes. LMK.

Github user vladimir-kotikov commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-217091665

LGTM

Commit 6025a5f23c00284fb3416d8042ff5d738bf03db3 in cordova-lib's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=6025a5f ]

CB-9858 merging initial fetch work for plugin and platform fetching

Commit 96c522f0ce3901a2364c0a648a62e36dc4dd7a45 in cordova-cli's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=96c522f ]

CB-9858 added --fetch option

Commit d19111506f870eb92d783976a371996071792c5c in cordova-plugman's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-plugman.git;h=d191115 ]

CB-9858 added --fetch option to plugman

Github user stevengill commented on the pull request:

https://github.com/apache/cordova-lib/pull/407#issuecomment-218000066

Merged

Github user stevengill closed the pull request at:

https://github.com/apache/cordova-lib/pull/407

Commit 5e2149feec7139ab24439b5f6d22b5763d278242 in cordova-cli's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=5e2149f ]

CB-9858 added --fetch to cli docs

Commit 18e7377b9ef64c67a80fc8ea754e5bc9d7636be8 in cordova-cli's branch refs/heads/master from Vladimir Kotikov
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=18e7377 ]

CB-9858 Fix tests failing due to new --fetch option

Commit 773d2a8b679234f4189f0017f89db64c68d8bb90 in cordova-cli's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=773d2a8 ]

CB-9858 added --fetch to readme

Commit be7eb6edb56de7022f00af53d79aa6705c0d2f66 in cordova-coho's branch refs/heads/master from Vladimir Kotikov
[ https://git-wip-us.apache.org/repos/asf?p=cordova-coho.git;h=be7eb6e ]

CB-9858 Implement Cordova Fetch Proposal

Ensure fetch and cli is properly installed/linked
Add fetch to list of tools repos
Resolve cordova-fetch location properly when npm-linking modules

Commit 2aac4434486278379f22ca0715eec5f506401671 in cordova-lib's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=2aac443 ]

CB-9858 fixed failing travis and appveyor tests

Commit f1e34f9b5113065a0ed49f05423e93bc3cd24c93 in cordova-lib's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=f1e34f9 ]

CB-9858 added fetch tests to travis

Commit 83d3383dc66d069ae902f100a407031f57bf93be in cordova-cli's branch refs/heads/master from Steve Gill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=83d3383 ]

CB-9858 fixed failing appveyor and travis builds

Commit 8f1e51d366a2faa7a69335d503265c54e18771c8 in cordova-coho's branch refs/heads/master from Vladimir Kotikov
[ https://git-wip-us.apache.org/repos/asf?p=cordova-coho.git;h=8f1e51d ]

CB-9858 Link cordova-common to cordova-fetch

Commit 9e98cda186e91b53736a9e4d9ee99e09a0eec5ec in cordova-coho's branch refs/heads/master from Vladimir Kotikov
[ https://git-wip-us.apache.org/repos/asf?p=cordova-coho.git;h=9e98cda ]

CB-9858 Set up links for cordova-common properly

Github user purplecabbage commented on the issue:

https://github.com/apache/cordova-cli/pull/239

Is this still valid? Needs a rebase at least ...

Github user stevengill commented on the issue:

https://github.com/apache/cordova-cli/pull/239

This got merged in already. Don't know why it didn't close.

Github user stevengill closed the pull request at:

https://github.com/apache/cordova-cli/pull/239