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

Remove lazy loading of node modules from cordova tooling

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: cordova-common, cordova-lib
    • Labels:
      None

      Description

      There are various addProperty* methods that execute require at runtime. While this improves load time and only loads modules that are necessary, it also makes it harder to test, as spying and mocking is more difficult.

      The 300ms load time improvement, in the best case, is not worth the difficulty in writing unit tests.

        Issue Links

          Activity

          Hide
          filmaj Filip Maj added a comment -

          FYI I have a branch with this in progress here: https://github.com/filmaj/cordova-lib/tree/no-lazy-load

          Show
          filmaj Filip Maj added a comment - FYI I have a branch with this in progress here: https://github.com/filmaj/cordova-lib/tree/no-lazy-load
          Hide
          filmaj Filip Maj added a comment -

          Unfortunately the simple removal of this functionality prompted a refactor, which in turn prompted tests to fail! So this branch is a larger work in progress in further refactoring the platform command, and laying out a structure for its unit tests.

          Show
          filmaj Filip Maj added a comment - Unfortunately the simple removal of this functionality prompted a refactor, which in turn prompted tests to fail! So this branch is a larger work in progress in further refactoring the platform command, and laying out a structure for its unit tests.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user stevengill opened a pull request:

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

          CB-12901: removed .raw from cordova-lib calls

          <!--
          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?
              1. What testing has been done on this change?
              1. Checklist
          • [ ] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
          • [ ] 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/stevengill/cordova-cli CB-12901

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

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


          commit a4c71847702793fc24cdfe393f954e4b2f1fc863
          Author: Steve Gill <stevengill97@gmail.com>
          Date: 2017-06-16T21:37:07Z

          CB-12901: removed .raw from cordova-lib calls


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user stevengill opened a pull request: https://github.com/apache/cordova-cli/pull/282 CB-12901 : removed .raw from cordova-lib calls <!-- 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? What testing has been done on this change? Checklist [ ] [Reported an issue] ( http://cordova.apache.org/contribute/issues.html ) in the JIRA database [ ] 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/stevengill/cordova-cli CB-12901 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-cli/pull/282.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 #282 commit a4c71847702793fc24cdfe393f954e4b2f1fc863 Author: Steve Gill <stevengill97@gmail.com> Date: 2017-06-16T21:37:07Z CB-12901 : removed .raw from cordova-lib calls
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user purplecabbage opened a pull request:

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

          CB-12901 Cli Refactor

              1. What does this PR do?
          • fixed failing tests.
          • adjusted tests to call cordova instead of cordova.raw
          • added tests to compare cordova.raw and cordova
          • changed some console.log to logger.log
          • help module returns help text and does not print it
            • this is to make testing easier and not have it spit all over jasmine output.
          • various minor refactors, removed some un-needed promises etc.
          • added timeoutInSecs property to make user prompts testable without waiting for 30 seconds
          • this includes @stevengill 's previous pr
              1. Platforms affected
                tooling
              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/purplecabbage/cordova-cli RefactorWithCB-12901

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

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


          commit a4c71847702793fc24cdfe393f954e4b2f1fc863
          Author: Steve Gill <stevengill97@gmail.com>
          Date: 2017-06-16T21:37:07Z

          CB-12901: removed .raw from cordova-lib calls

          commit a24ec7968f4b271d9c3487980d29b3766b690053
          Author: Jesse MacFadyen <purplecabbage@gmail.com>
          Date: 2017-06-20T05:17:56Z

          fixed lines that were too long, test was failing

          commit a26a73a420f71c972094513580921070a9026a47
          Author: Jesse MacFadyen <purplecabbage@gmail.com>
          Date: 2017-06-20T05:19:06Z

          added timeoutInSecs property to make user prompts testable without waiting for 30 seconds

          commit 310d6b0a1c33d2289b8909fc61d13b46c7ca5950
          Author: Jesse MacFadyen <purplecabbage@gmail.com>
          Date: 2017-06-20T05:22:13Z

          adjusted tests to call cordova instead of cordova.raw, added tests to compare cordova.raw and cordova, changed some console.log to logger.log; help module returns help text and does not print it, this is to make testing easier and not have it spit all over jasmine output. various minor refactors, removed some un-needed promises


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user purplecabbage opened a pull request: https://github.com/apache/cordova-cli/pull/283 CB-12901 Cli Refactor What does this PR do? fixed failing tests. adjusted tests to call cordova instead of cordova.raw added tests to compare cordova.raw and cordova changed some console.log to logger.log help module returns help text and does not print it this is to make testing easier and not have it spit all over jasmine output. various minor refactors, removed some un-needed promises etc. added timeoutInSecs property to make user prompts testable without waiting for 30 seconds this includes @stevengill 's previous pr Platforms affected tooling 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/purplecabbage/cordova-cli RefactorWithCB-12901 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-cli/pull/283.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 #283 commit a4c71847702793fc24cdfe393f954e4b2f1fc863 Author: Steve Gill <stevengill97@gmail.com> Date: 2017-06-16T21:37:07Z CB-12901 : removed .raw from cordova-lib calls commit a24ec7968f4b271d9c3487980d29b3766b690053 Author: Jesse MacFadyen <purplecabbage@gmail.com> Date: 2017-06-20T05:17:56Z fixed lines that were too long, test was failing commit a26a73a420f71c972094513580921070a9026a47 Author: Jesse MacFadyen <purplecabbage@gmail.com> Date: 2017-06-20T05:19:06Z added timeoutInSecs property to make user prompts testable without waiting for 30 seconds commit 310d6b0a1c33d2289b8909fc61d13b46c7ca5950 Author: Jesse MacFadyen <purplecabbage@gmail.com> Date: 2017-06-20T05:22:13Z adjusted tests to call cordova instead of cordova.raw, added tests to compare cordova.raw and cordova, changed some console.log to logger.log; help module returns help text and does not print it, this is to make testing easier and not have it spit all over jasmine output. various minor refactors, removed some un-needed promises
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user alsorokin commented on the issue:

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

          Without these changes `cordova create foo` would give us `Error: cordova.raw.create is not a function`, right?
          Or is it my local linking issue?

          Show
          githubbot ASF GitHub Bot added a comment - Github user alsorokin commented on the issue: https://github.com/apache/cordova-cli/pull/283 Without these changes `cordova create foo` would give us `Error: cordova.raw.create is not a function`, right? Or is it my local linking issue?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user purplecabbage commented on the issue:

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

          That is correct!
          The cordova-lib changes are already on master, so together they are broken without these changes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user purplecabbage commented on the issue: https://github.com/apache/cordova-cli/pull/283 That is correct! The cordova-lib changes are already on master, so together they are broken without these changes.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user alsorokin commented on the issue:

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

          @purplecabbage great, thanks for the clarification

          Show
          githubbot ASF GitHub Bot added a comment - Github user alsorokin commented on the issue: https://github.com/apache/cordova-cli/pull/283 @purplecabbage great, thanks for the clarification
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          CB-12901: removed .raw from cordova-lib calls

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0a42092971dc8fe2f483bd42c3b9de26fdec677c in cordova-cli's branch refs/heads/master from Steve Gill [ https://git-wip-us.apache.org/repos/asf?p=cordova-cli.git;h=0a42092 ] CB-12901 : removed .raw from cordova-lib calls
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user asfgit closed the pull request at:

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

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

            People

            • Assignee:
              filmaj Filip Maj
              Reporter:
              filmaj Filip Maj
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development