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

add deprecation notice for using subdirectory syntax when fetching plugins

    Details

      Description

      We plan to remove subdirectory syntax in cordova@7 since `--fetch` can't support it.

      Need to add a deprecation to the next cordova@6.x release to start warning folks it is going away.

      You can see the subdirectory syntax at http://cordova.apache.org/docs/en/latest/reference/cordova-cli/index.html#plugin-spec.

      I think it is plugin specific, but need to double check it is not supported for fetching platforms too.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user sterlingann opened a pull request:

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

          CB-11979 added deprecation warning for subdirectories

          <!--
          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
                all
              1. What does this PR do?
                warns of deprecation of subdirectories in Cordova@7
              1. What testing has been done on this change?
                added and removed a subdirectory plugin, saw warning
              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.
          • [ ] 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/sterlingann/cordova-lib CB-11979

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

          https://github.com/apache/cordova-lib/pull/504.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 #504


          commit 497a6d57f5c4ebb324ce6a838dd96a5881a21a75
          Author: Sterling <sterlingann@gmail.com>
          Date: 2016-10-18T22:29:09Z

          CB-11979 added deprecation warning for subdirectories


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user sterlingann opened a pull request: https://github.com/apache/cordova-lib/pull/504 CB-11979 added deprecation warning for subdirectories <!-- 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 all What does this PR do? warns of deprecation of subdirectories in Cordova@7 What testing has been done on this change? added and removed a subdirectory plugin, saw warning 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. [ ] 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/sterlingann/cordova-lib CB-11979 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/504.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 #504 commit 497a6d57f5c4ebb324ce6a838dd96a5881a21a75 Author: Sterling <sterlingann@gmail.com> Date: 2016-10-18T22:29:09Z CB-11979 added deprecation warning for subdirectories
          Hide
          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/504#discussion_r83974283

          — Diff: cordova-lib/src/plugman/fetch.js —
          @@ -70,6 +70,10 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
          //if --fetch was used, throw error for subdirectories
          if (result[2] && options.fetch)

          { return Q.reject(new CordovaError('--fetch does not support subdirectories')); + }

          else {
          + if(result[2])

          { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); + }

          — End diff –

          if fetch and subdirectory are both present, it will just do a straight reject return Q.reject(new CordovaError('--fetch does not support subdirectories'));

          Show
          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/504#discussion_r83974283 — Diff: cordova-lib/src/plugman/fetch.js — @@ -70,6 +70,10 @@ function fetchPlugin(plugin_src, plugins_dir, options) { //if --fetch was used, throw error for subdirectories if (result [2] && options.fetch) { return Q.reject(new CordovaError('--fetch does not support subdirectories')); + } else { + if(result [2] ) { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); + } — End diff – if fetch and subdirectory are both present, it will just do a straight reject return Q.reject(new CordovaError('--fetch does not support subdirectories'));
          Hide
          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/504#discussion_r83974505

          — Diff: cordova-lib/src/plugman/fetch.js —
          @@ -70,6 +70,10 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
          //if --fetch was used, throw error for subdirectories
          if (result[2] && options.fetch)

          { return Q.reject(new CordovaError('--fetch does not support subdirectories')); + }

          else {
          + if(result[2])

          { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); + }

          — End diff –

          so, if fetch and subdir -> fail, if subdir && no fetch -> do deprecation notice. If we move it up, users who are using --fetch get deprecation warning & failure. That would be acceptable too.

          Show
          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/504#discussion_r83974505 — Diff: cordova-lib/src/plugman/fetch.js — @@ -70,6 +70,10 @@ function fetchPlugin(plugin_src, plugins_dir, options) { //if --fetch was used, throw error for subdirectories if (result [2] && options.fetch) { return Q.reject(new CordovaError('--fetch does not support subdirectories')); + } else { + if(result [2] ) { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); + } — End diff – so, if fetch and subdir -> fail, if subdir && no fetch -> do deprecation notice. If we move it up, users who are using --fetch get deprecation warning & failure. That would be acceptable too.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user codecov-io commented on the issue:

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

            1. [Current coverage](https://codecov.io/gh/apache/cordova-lib/pull/504?src=pr) is 80.62% (diff: 100%)
              > Merging 504(https://codecov.io/gh/apache/cordova-lib/pull/504?src=pr) into [master](https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr) will increase coverage by *<.01%*

          ```diff
          @@ master #504 diff @@
          ==========================================
          Files 67 67
          Lines 5215 5217 +2
          Methods 843 843
          Messages 0 0
          Branches 1010 1011 +1
          ==========================================
          + Hits 4204 4206 +2
          Misses 1011 1011
          Partials 0 0
          ```

          > Powered by [Codecov](https://codecov.io?src=pr). Last update [a170a46...497a6d5](https://codecov.io/gh/apache/cordova-lib/compare/a170a465ec0a2dc4bbc98bfa47e03041521cf22e...497a6d57f5c4ebb324ce6a838dd96a5881a21a75?src=pr)

          Show
          githubbot ASF GitHub Bot added a comment - Github user codecov-io commented on the issue: https://github.com/apache/cordova-lib/pull/504 [Current coverage] ( https://codecov.io/gh/apache/cordova-lib/pull/504?src=pr ) is 80.62% (diff: 100%) > Merging 504 ( https://codecov.io/gh/apache/cordova-lib/pull/504?src=pr ) into [master] ( https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr ) will increase coverage by * <.01% * ```diff @@ master #504 diff @@ ========================================== Files 67 67 Lines 5215 5217 +2 Methods 843 843 Messages 0 0 Branches 1010 1011 +1 ========================================== + Hits 4204 4206 +2 Misses 1011 1011 Partials 0 0 ``` > Powered by [Codecov] ( https://codecov.io?src=pr ). Last update [a170a46...497a6d5] ( https://codecov.io/gh/apache/cordova-lib/compare/a170a465ec0a2dc4bbc98bfa47e03041521cf22e...497a6d57f5c4ebb324ce6a838dd96a5881a21a75?src=pr )
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shazron commented on the issue:

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

          Make sure to run `npm test` before re-submitting your changes. CI fail due to jshint error.
          `src/plugman/fetch.js: line 72, col 12, 'option' is not defined.`

          Show
          githubbot ASF GitHub Bot added a comment - Github user shazron commented on the issue: https://github.com/apache/cordova-lib/pull/504 Make sure to run `npm test` before re-submitting your changes. CI fail due to jshint error. `src/plugman/fetch.js: line 72, col 12, 'option' is not defined.`
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/cordova-lib/pull/504#discussion_r84220511

          — Diff: cordova-lib/src/plugman/fetch.js —
          @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
          if (result[2])
          options.subdir = result[2];
          //if --fetch was used, throw error for subdirectories

          • if (result[2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - }

            +
            + if(options.subdir) {
            + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7');
            + if (options.fetch) {

              • End diff –

          Probably an Indentation issue? This should has the same level of indentation as line above

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/504#discussion_r84220511 — Diff: cordova-lib/src/plugman/fetch.js — @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) { if (result [2] ) options.subdir = result [2] ; //if --fetch was used, throw error for subdirectories if (result [2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - } + + if(options.subdir) { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); + if (options.fetch) { End diff – Probably an Indentation issue? This should has the same level of indentation as line above
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/cordova-lib/pull/504#discussion_r84220720

          — Diff: cordova-lib/src/plugman/fetch.js —
          @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
          if (result[2])
          options.subdir = result[2];
          //if --fetch was used, throw error for subdirectories

          • if (result[2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - }

            +
            + if(options.subdir) {
            + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7');

              • End diff –

          typo: 'depreated' -> 'deprecated'

          Show
          githubbot ASF GitHub Bot added a comment - Github user vladimir-kotikov commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/504#discussion_r84220720 — Diff: cordova-lib/src/plugman/fetch.js — @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) { if (result [2] ) options.subdir = result [2] ; //if --fetch was used, throw error for subdirectories if (result [2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - } + + if(options.subdir) { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); End diff – typo: 'depreated' -> 'deprecated'
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/cordova-lib/pull/504#discussion_r84335445

          — Diff: cordova-lib/src/plugman/fetch.js —
          @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
          if (result[2])
          options.subdir = result[2];
          //if --fetch was used, throw error for subdirectories

          • if (result[2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - }

            +
            + if(options.subdir) {
            + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7');

              • End diff –

          Thank you Vladimir! I'm an intern, this is my first pull request. I appreciate the feedback I will fix it now...

          Show
          githubbot ASF GitHub Bot added a comment - Github user sterlingann commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/504#discussion_r84335445 — Diff: cordova-lib/src/plugman/fetch.js — @@ -68,9 +68,13 @@ function fetchPlugin(plugin_src, plugins_dir, options) { if (result [2] ) options.subdir = result [2] ; //if --fetch was used, throw error for subdirectories if (result [2] && options.fetch) { - return Q.reject(new CordovaError('--fetch does not support subdirectories')); - } + + if(options.subdir) { + events.emit('warn', 'support for subdirectories is depreated and will be removed in Cordova@7'); End diff – Thank you Vladimir! I'm an intern, this is my first pull request. I appreciate the feedback I will fix it now...
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 328980c2a91dada8049f1e4f27103df67ba07532 in cordova-lib's branch refs/heads/master from sterling gerritz
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=328980c ]

          CB-11979 added deprecation warning for subdirectories

          Show
          jira-bot ASF subversion and git services added a comment - Commit 328980c2a91dada8049f1e4f27103df67ba07532 in cordova-lib's branch refs/heads/master from sterling gerritz [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=328980c ] CB-11979 added deprecation warning for subdirectories
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user stevengill commented on the issue:

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

          This PR can be closed now! It was merged

          Show
          githubbot ASF GitHub Bot added a comment - Github user stevengill commented on the issue: https://github.com/apache/cordova-lib/pull/504 This PR can be closed now! It was merged
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sterlingann closed the pull request at:

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

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

          Github user yyfearth commented on the issue:

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

          This fix has issue now since
          ```
          options.subdir = options.subdir || '.';
          ...
          if (result[2]) options.subdir = result[2]; // not work if it is empty
          ...
          if(options.subdir) { // it become always true, since subdir will be "."
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user yyfearth commented on the issue: https://github.com/apache/cordova-lib/pull/504 This fix has issue now since ``` options.subdir = options.subdir || '.'; ... if (result [2] ) options.subdir = result [2] ; // not work if it is empty ... if(options.subdir) { // it become always true, since subdir will be "." ```
          Hide
          yyfearth@gmail.com Wilson Yang added a comment - - edited

          I have create another PR to fix this issue I mentioned above in CB-12261.

          Show
          yyfearth@gmail.com Wilson Yang added a comment - - edited I have create another PR to fix this issue I mentioned above in CB-12261 .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user yyfearth commented on the issue:

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

          I have create RP #511 CB-12261 to fix the issue.

          Show
          githubbot ASF GitHub Bot added a comment - Github user yyfearth commented on the issue: https://github.com/apache/cordova-lib/pull/504 I have create RP #511 CB-12261 to fix the issue.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4191fa9b25b06140c58037ab9b504d6c9b9b064d in cordova-lib's branch refs/heads/master from Wilson Yang
          [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=4191fa9 ]

          CB-12261: fix subdirectories deprecated warning always shows and stop fetch caused by CB-11979

          This closes #511

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4191fa9b25b06140c58037ab9b504d6c9b9b064d in cordova-lib's branch refs/heads/master from Wilson Yang [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=4191fa9 ] CB-12261 : fix subdirectories deprecated warning always shows and stop fetch caused by CB-11979 This closes #511

            People

            • Assignee:
              sterlingann sterling gerritz
              Reporter:
              stevegill Steve Gill
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development