Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
None
Description
cordova platform add android --save --fetch should use the version of cordova-android already saved in package.json instead of the version from platformsConfig.json
Attachments
Issue Links
- links to
Activity
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-lib/pull/518
- [Codecov](https://codecov.io/gh/apache/cordova-lib/pull/518?src=pr&el=h1) Report
> Merging 518(https://codecov.io/gh/apache/cordova-lib/pull/518?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-lib/commit/a9605411ab007d700849c971043f0c211c06100c?src=pr&el=desc) will *increase* coverage by `0.17%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
-
- master #518 +/- ##
==========================================
+ Coverage 81.8% 81.98% +0.17%
==========================================
Files 68 68
Lines 5459 5500 +41
Branches 1072 1086 +14
==========================================
+ Hits 4466 4509 +43
+ Misses 993 991 -2
```
- master #518 +/- ##
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-lib/pull/518?src=pr&el=continue).
> *Legend* - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-lib/pull/518?src=pr&el=footer). Last update [a960541...f8b4483](https://codecov.io/gh/apache/cordova-lib/compare/a9605411ab007d700849c971043f0c211c06100c...f8b4483150c3c42029547b583117042295a4113d?src=pr&el=footer&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101826675
— Diff: cordova-lib/src/plugman/fetch.js —
@@ -69,7 +71,7 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
options.subdir = result[2];
//if --fetch was used, throw error for subdirectories
- if (options.subdir && options.subdir !== '.') {
-
- End diff –
-
I think this may have been taken out by mistake
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101873081
— Diff: cordova-lib/spec-cordova/platform.spec.js —
@@ -353,4 +353,5 @@ describe('plugin add and rm end-to-end --fetch', function () {
})
.fin(done);
}, 60000);
-});
\ No newline at end of file
+});
+
— End diff –
nit: delete extra new line
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101872753
— Diff: cordova-lib/src/cordova/platform.js —
@@ -253,6 +275,7 @@ function addHelper(cmd, hooksRunner, projectRoot, targets, opts) {
if(fs.existsSync(pkgJsonPath)) {
delete require.cache[require.resolve(pkgJsonPath)];
pkgJson = require(pkgJsonPath);
+
— End diff –
nit: unnecessary line
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101873667
— Diff: cordova-lib/spec-cordova/pkgJson-restore.spec.js —
@@ -742,7 +742,7 @@ describe('update config.xml to use the variable found in pkg.json', function ()
process.chdir(path.join(__dirname, '..')); // Needed to rm the dir on Windows.
shell.rm('-rf', tmpDir);
});
-
+
— End diff –
nit: unnecessary tab
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101870795
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -388,26 +384,54 @@ module.exports = function plugin(command, targets, opts) {
function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
var parsedSpec = pluginSpec.parse(target);
-
var id = parsedSpec.package || target;
-
// CB-10975 We need to resolve relative path to plugin dir from app's root before checking whether if it exists
var maybeDir = cordova_util.fixRelativePath(id);
if (parsedSpec.version || cordova_util.isUrl(id) || cordova_util.isDirectory(maybeDir))
+ // Require project pkgJson.
+ var pkgJsonPath = path.join(projectRoot, 'package.json');
+ if(fs.existsSync(pkgJsonPath))
- // If no version is specified, retrieve the version (or source) from config.xml
- events.emit('verbose', 'No version specified for ' + parsedSpec.package + ', retrieving version from config.xml');
- var ver = getVersionFromConfigFile(id, cfg);
+ // If no parsedSpec.version, use the one from pkg.json or config.xml.
+ if (!parsedSpec.version)Unknown macro: { + // Retrieve from pkg.json. + if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[id]) { + events.emit('verbose', 'No version specified for ' + id + ', retrieving version from package.json'); + parsedSpec.version = pkgJson.dependencies[id]; + } else { + // If no version is specified, retrieve the version (or source) from config.xml. + events.emit('verbose', 'No version specified for ' + id + ', retrieving version from config.xml'); + parsedSpec.version = getVersionFromConfigFile(id, cfg); + } + }+
{ + noSymbolVersion = parsedSpec.version.slice(1); + }
+ // If parsedSpec.version satisfies pkgJson version, no writing to pkg.json. Only write when
+ // it does not satisfy.
+ if(parsedSpec.version) {
+ if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
+ var noSymbolVersion;
+ if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~')+ if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
-
- End diff –
-
`noSymbolVersion` doesn't have a value if `parsedSpec.version` doesn't have a `^` or `~`. I'd fix that by giving `noSymbolVersion` a default value of `parsedSpec.version`. Like:
```
var noSymbolVersion = parsedSpec.version
```
So the purpose of this code is to replace the plugin dependency version in `package.json` if the user passes in a specific one that doesn't satisfy what is already in `package.json`.
I believe `parsedSpec.version` can be:
- a version number (possibly with ^ or ~)
- a url
- a local path
I think semver.satisfies would fail if `parsedSpec.version` was url or path.
Maybe we do checks similar to line 428 to see if `parsedSpec.version` is a URL or directory. If it is, don't do th satisfies check, instead do a equivalence check (pkgJson.dependencies[parsedSpec.package] === parsedSpec.version)
We also don't check if fetchOptions.save is true. We don't want to write to package.json unless that is true.
Github user audreyso commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101875939
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -388,26 +384,54 @@ module.exports = function plugin(command, targets, opts) {
function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
var parsedSpec = pluginSpec.parse(target);
-
var id = parsedSpec.package || target;
-
// CB-10975 We need to resolve relative path to plugin dir from app's root before checking whether if it exists
var maybeDir = cordova_util.fixRelativePath(id);
if (parsedSpec.version || cordova_util.isUrl(id) || cordova_util.isDirectory(maybeDir))
+ // Require project pkgJson.
+ var pkgJsonPath = path.join(projectRoot, 'package.json');
+ if(fs.existsSync(pkgJsonPath))
- // If no version is specified, retrieve the version (or source) from config.xml
- events.emit('verbose', 'No version specified for ' + parsedSpec.package + ', retrieving version from config.xml');
- var ver = getVersionFromConfigFile(id, cfg);
+ // If no parsedSpec.version, use the one from pkg.json or config.xml.
+ if (!parsedSpec.version)Unknown macro: { + // Retrieve from pkg.json. + if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[id]) { + events.emit('verbose', 'No version specified for ' + id + ', retrieving version from package.json'); + parsedSpec.version = pkgJson.dependencies[id]; + } else { + // If no version is specified, retrieve the version (or source) from config.xml. + events.emit('verbose', 'No version specified for ' + id + ', retrieving version from config.xml'); + parsedSpec.version = getVersionFromConfigFile(id, cfg); + } + }+
{ + noSymbolVersion = parsedSpec.version.slice(1); + }
+ // If parsedSpec.version satisfies pkgJson version, no writing to pkg.json. Only write when
+ // it does not satisfy.
+ if(parsedSpec.version) {
+ if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
+ var noSymbolVersion;
+ if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~')+ if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
-
- End diff –
-
How does this look?
```// If parsedSpec.version satisfies pkgJson version, no writing to pkg.json. Only write when
// it does not satisfy.
if(parsedSpec.version) {
if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
var noSymbolVersion = parsedSpec.version;
if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~')
if (pkgJson.dependencies[parsedSpec.package] === parsedSpec.version) {
pkgJson.dependencies[parsedSpec.package] = parsedSpec.version;
if (fetchOptions.save === true)
}
}
}
if (cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version) || pluginSpec.parse(parsedSpec.version).scope)
{ return Q(parsedSpec.version); }```
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101877994
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -414,13 +414,15 @@ function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
// it does not satisfy.
if(parsedSpec.version) {
if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
- var noSymbolVersion;
+ var noSymbolVersion = parsedSpec.version;
if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~') { noSymbolVersion = parsedSpec.version.slice(1); } - if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
+ if (pkgJson.dependencies[parsedSpec.package] === parsedSpec.version) {-
- End diff –
-
I think we still want to do the semver check too. First start with outer if that checks if `parsedSpec.version` is a directory or url, if it is, do this check to see if `pkgJson.dependencies[parsedSpec.package] !== parsedSpec.version`. If that is true, then you want to update package.json (assuming fetchOpts.save === true)
On that outer if that checks if it is a dir or url, add a else if where you check for semver.satisfies
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r101877578
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -414,13 +414,15 @@ function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
// it does not satisfy.
if(parsedSpec.version) {
if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
- var noSymbolVersion;
+ var noSymbolVersion = parsedSpec.version;
if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~') { noSymbolVersion = parsedSpec.version.slice(1); } - if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
+ if (pkgJson.dependencies[parsedSpec.package] === parsedSpec.version) {
pkgJson.dependencies[parsedSpec.package] = parsedSpec.version; - fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, 4), 'utf8');
+ if (fetchOptions.save === true) {-
- End diff –
-
this is good!
Github user audreyso commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r102099272
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -412,15 +412,26 @@ function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
// If parsedSpec.version satisfies pkgJson version, no writing to pkg.json. Only write when
// it does not satisfy.
+
if(parsedSpec.version) {
if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
- var noSymbolVersion;
+ var noSymbolVersion = parsedSpec.version;
if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~') { noSymbolVersion = parsedSpec.version.slice(1); } - if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
+
+ if(cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version)) {-
- End diff –
-
How does this look (lines 423- 437)? @stevengill
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/518#discussion_r102321817
— Diff: cordova-lib/src/cordova/plugin.js —
@@ -412,15 +412,26 @@ function determinePluginTarget(projectRoot, cfg, target, fetchOptions) {
// If parsedSpec.version satisfies pkgJson version, no writing to pkg.json. Only write when
// it does not satisfy.
+
if(parsedSpec.version) {
if(pkgJson && pkgJson.dependencies && pkgJson.dependencies[parsedSpec.package]) {
- var noSymbolVersion;
+ var noSymbolVersion = parsedSpec.version;
if (parsedSpec.version.charAt(0) === '^' || parsedSpec.version.charAt(0) === '~') { noSymbolVersion = parsedSpec.version.slice(1); } - if (!semver.satisfies(noSymbolVersion, pkgJson.dependencies[parsedSpec.package])) {
+
+ if(cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version)) {-
- End diff –
-
LGTM!
Github user stevengill commented on the issue:
https://github.com/apache/cordova-lib/pull/518
I think this is ready to merge in
Commit b3ca300172fdb94860a523505ab12d90512d984c in cordova-lib's branch refs/heads/master from stevegill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b3ca300 ]
CB-12021: Added local path support to --fetch and fixed failing tests for adding a relative path
Commit a5b0441d5572a9c37b235e33bd6d7af3ce9ffc03 in cordova-lib's branch refs/heads/master from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a5b0441 ]
CB-12021 : updated platform.js to fix failing tests and modified test 25 to check for config.xml too
Commit 4d095776ce058dc52f74ef56912ff3cae6b28a49 in cordova-lib's branch refs/heads/master from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=4d09577 ]
CB-12021 : updated jshint and added includes method to support node 0.x
Commit b0402b9e51bf2d97488fedd39d13348b29f81c3c in cordova-lib's branch refs/heads/master from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b0402b9 ]
CB-12021 : updated final changes and fixed typos/whitespace after final review
This closes #518
Commit b3ca300172fdb94860a523505ab12d90512d984c in cordova-lib's branch refs/heads/common-2.0.x from stevegill
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b3ca300 ]
CB-12021: Added local path support to --fetch and fixed failing tests for adding a relative path
Commit a5b0441d5572a9c37b235e33bd6d7af3ce9ffc03 in cordova-lib's branch refs/heads/common-2.0.x from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=a5b0441 ]
CB-12021 : updated platform.js to fix failing tests and modified test 25 to check for config.xml too
Commit 4d095776ce058dc52f74ef56912ff3cae6b28a49 in cordova-lib's branch refs/heads/common-2.0.x from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=4d09577 ]
CB-12021 : updated jshint and added includes method to support node 0.x
Commit b0402b9e51bf2d97488fedd39d13348b29f81c3c in cordova-lib's branch refs/heads/common-2.0.x from auso
[ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=b0402b9 ]
CB-12021 : updated final changes and fixed typos/whitespace after final review
This closes #518
GitHub user audreyso opened a pull request:
https://github.com/apache/cordova-lib/pull/518
CB-12021: --fetch should use dependency version saved in package.json before default cordova version when adding/restoring<!--
Please make sure the checklist boxes are all checked before submitting the PR. The checklist
is intended as a quick reference, for complete details please see our Contributor Guidelines:
http://cordova.apache.org/contribute/contribute_guidelines.html
Thanks!
-->
--fetch should use dependency version saved in package.json before default cordova version when adding/restoring
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/audreyso/cordova-lib fixfetch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/518.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 #518
commit f448f2f6530222ff39f5b1658f9fede50df0720b
Author: audreyso <auso@adobe.com>
Date: 2016-10-07T21:19:06Z
CB-11960: Added support to package.json for platform/plugin add/rmcommit b3ca300172fdb94860a523505ab12d90512d984c
Author: Steve Gill <stevengill97@gmail.com>
Date: 2016-11-04T20:55:52Z
CB-12021: Added local path support to --fetch and fixed failing tests for adding a relative pathcommit e5efb1a92a10f3e179618354373b0a51975f6588
Author: Audrey So <audreyso@apache.org>
Date: 2017-02-15T18:10:32Z
fixfetch : updated index.js to deal with local path
commit a5b0441d5572a9c37b235e33bd6d7af3ce9ffc03
Author: Audrey So <audreyso@apache.org>
Date: 2017-02-16T02:03:25Z
CB-12021: updated platform.js to fix failing tests and modified test 25 to check for config.xml toocommit 4d095776ce058dc52f74ef56912ff3cae6b28a49
Author: Audrey So <audreyso@apache.org>
Date: 2017-02-16T23:40:53Z
CB-12021: updated jshint and added includes method to support node 0.xcommit c8bf8028efa84287c9876d44a94255233b9e0974
Author: Audrey So <audreyso@apache.org>
Date: 2017-02-17T00:23:40Z
CB-12021: adding cordova-browser node_modules