Uploaded image for project: 'Apache Cordova'
  1. Apache Cordova
  2. CB-12021

--fetch should use dependency version saved in package.json before default cordova version when adding/restoring

Details

    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

          Activity

            githubbot ASF GitHub Bot added a comment -

            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!
            -->

                1. Platforms affected
                1. What does this PR do?
                  --fetch should use dependency version saved in package.json before default cordova version when adding/restoring
                1. What testing has been done on this change?
                1. Checklist
            • [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
            • [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
            • [X] Added automated test coverage as appropriate for this change.

            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/rm

            commit 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 path

            commit 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 too

            commit 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.x

            commit c8bf8028efa84287c9876d44a94255233b9e0974
            Author: Audrey So <audreyso@apache.org>
            Date: 2017-02-17T00:23:40Z

            CB-12021 : adding cordova-browser node_modules


            githubbot ASF GitHub Bot added a comment - 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! --> Platforms affected What does this PR do? --fetch should use dependency version saved in package.json before default cordova version when adding/restoring What testing has been done on this change? Checklist [X] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. [X] Added automated test coverage as appropriate for this change. 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/rm commit 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 path commit 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 too commit 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.x commit c8bf8028efa84287c9876d44a94255233b9e0974 Author: Audrey So <audreyso@apache.org> Date: 2017-02-17T00:23:40Z CB-12021 : adding cordova-browser node_modules
            githubbot ASF GitHub Bot added a comment -

            Github user codecov-io commented on the issue:

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

            1. [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 @@

              1. 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
                ```

            ------

            [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).

            githubbot ASF GitHub Bot added a comment - 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 ``` [Impacted Files] ( https://codecov.io/gh/apache/cordova-lib/pull/518?src=pr&el=tree ) Coverage Δ   — — — [cordova-lib/src/cordova/plugin.js] ( https://codecov.io/gh/apache/cordova-lib/compare/a9605411ab007d700849c971043f0c211c06100c...c8bf8028efa84287c9876d44a94255233b9e0974?src=pr&el=tree#diff-Y29yZG92YS1saWIvc3JjL2NvcmRvdmEvcGx1Z2luLmpz ) `89.75% <ø> (-0.09%)` :x: [cordova-lib/src/cordova/platform.js] ( https://codecov.io/gh/apache/cordova-lib/compare/a9605411ab007d700849c971043f0c211c06100c...c8bf8028efa84287c9876d44a94255233b9e0974?src=pr&el=tree#diff-Y29yZG92YS1saWIvc3JjL2NvcmRvdmEvcGxhdGZvcm0uanM= ) `78.34% <ø> (+1.08%)` :white_check_mark: [cordova-lib/src/plugman/fetch.js] ( https://codecov.io/gh/apache/cordova-lib/compare/a9605411ab007d700849c971043f0c211c06100c...c8bf8028efa84287c9876d44a94255233b9e0974?src=pr&el=tree#diff-Y29yZG92YS1saWIvc3JjL3BsdWdtYW4vZmV0Y2guanM= ) `81.46% <ø> (+2.04%)` :white_check_mark: [cordova-lib/src/cordova/platform_metadata.js] ( https://codecov.io/gh/apache/cordova-lib/compare/a9605411ab007d700849c971043f0c211c06100c...c8bf8028efa84287c9876d44a94255233b9e0974?src=pr&el=tree#diff-Y29yZG92YS1saWIvc3JjL2NvcmRvdmEvcGxhdGZvcm1fbWV0YWRhdGEuanM= ) `88.88% <ø> (+2.22%)` :white_check_mark: ------ [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 ).
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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))

            { return Q(target); }

            + // Require project pkgJson.
            + var pkgJsonPath = path.join(projectRoot, 'package.json');
            + if(fs.existsSync(pkgJsonPath))

            { + delete require.cache[require.resolve(pkgJsonPath)]; + pkgJson = require(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); + } + }

              +
              + // 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) === '~')

              { + noSymbolVersion = parsedSpec.version.slice(1); + }

              + 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.

            githubbot ASF GitHub Bot added a comment - 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)) { return Q(target); } + // Require project pkgJson. + var pkgJsonPath = path.join(projectRoot, 'package.json'); + if(fs.existsSync(pkgJsonPath)) { + delete require.cache[require.resolve(pkgJsonPath)]; + pkgJson = require(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); + } + } + + // 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) === '~') { + noSymbolVersion = parsedSpec.version.slice(1); + } + 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.
            githubbot ASF GitHub Bot added a comment -

            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))

            { return Q(target); }

            + // Require project pkgJson.
            + var pkgJsonPath = path.join(projectRoot, 'package.json');
            + if(fs.existsSync(pkgJsonPath))

            { + delete require.cache[require.resolve(pkgJsonPath)]; + pkgJson = require(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); + } + }

              +
              + // 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) === '~')

              { + noSymbolVersion = parsedSpec.version.slice(1); + }

              + 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) === '~')

            { noSymbolVersion = parsedSpec.version.slice(1); }

            if (pkgJson.dependencies[parsedSpec.package] === parsedSpec.version) {
            pkgJson.dependencies[parsedSpec.package] = parsedSpec.version;
            if (fetchOptions.save === true)

            { fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, 4), 'utf8'); }

            }
            }
            }

            if (cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version) || pluginSpec.parse(parsedSpec.version).scope)

            { return Q(parsedSpec.version); }

            ```

            githubbot ASF GitHub Bot added a comment - 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)) { return Q(target); } + // Require project pkgJson. + var pkgJsonPath = path.join(projectRoot, 'package.json'); + if(fs.existsSync(pkgJsonPath)) { + delete require.cache[require.resolve(pkgJsonPath)]; + pkgJson = require(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); + } + } + + // 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) === '~') { + noSymbolVersion = parsedSpec.version.slice(1); + } + 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) === '~') { noSymbolVersion = parsedSpec.version.slice(1); } if (pkgJson.dependencies [parsedSpec.package] === parsedSpec.version) { pkgJson.dependencies [parsedSpec.package] = parsedSpec.version; if (fetchOptions.save === true) { fs.writeFileSync(pkgJsonPath, JSON.stringify(pkgJson, null, 4), 'utf8'); } } } } if (cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version) || pluginSpec.parse(parsedSpec.version).scope) { return Q(parsedSpec.version); } ```
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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!

            githubbot ASF GitHub Bot added a comment - 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!
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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!

            githubbot ASF GitHub Bot added a comment - 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!
            githubbot ASF GitHub Bot added a comment -

            Github user stevengill commented on the issue:

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

            I think this is ready to merge in

            githubbot ASF GitHub Bot added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

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

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-lib/pull/518

            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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            jira-bot ASF subversion and git services added a comment - 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

            People

              auso Audrey So
              stevegill Steve Gill
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: