Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Android, CordovaLib
    • Labels:
      None

      Description

      We can delay load some of our dependencies to improve our load time.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user nikhilkh opened a pull request:

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

        CB-11194 Improve cordova load time

        I instrumented module load and this change improves our load time by ~50% (300+ ms) for `cordova build android` on a hello world project. Biggest gains are from delay loading only when required:

        • browserify
        • unorm
        • unpack

        ```diff
        — a/Users/nikhil/Developer/cordova/require.log
        +++ b/Users/nikhil/Developer/cordova/require-after.log
        @@ -1,116 +1,64 @@
        > Module load underscore: 4
        ->>> Module load graceful-fs: 3
        ->>> Module load osenv: 3
        ->>> Module load write-file-atomic: 4
        ->> Module load configstore: 17
        ->>>> Module load color-convert: 4
        +>> Module load child_process: 3
        +>>> Module load graceful-fs: 6
        +>>> Module load osenv: 4
        +>>>>> Module load crypto: 4
        +>>>> Module load ./rng: 5
        +>>> Module load uuid: 6
        +>>> Module load write-file-atomic: 3
        +>> Module load configstore: 24
        +>>>> Module load color-convert: 5
        >>> Module load ansi-styles: 6
        ->> Module load chalk: 11
        ->> Module load semver-diff: 3
        -> Module load update-notifier: 36
        ->> Module load ../hooks/HooksRunner: 3
        -> Module load ./build: 3
        ->>> Module load unorm: 24
        ->> Module load ../platforms/PlatformApiPoly: 30
        ->>>>>>>>>>>> Module load graceful-fs: 12
        ->>>>>>>>>>> Module load ./lib/reader.js: 13
        ->>>>>>>>>>>>>> Module load minimatch: 3
        ->>>>>>>>>>>>>> Module load inflight: 3
        ->>>>>>>>>>>>> Module load glob: 9
        ->>>>>>>>>>>> Module load rimraf: 10
        ->>>>>>>>>>> Module load ./lib/writer.js: 15
        ->>>>>>>>>> Module load fstream: 33
        ->>>>>>>>> Module load ./entry.js: 35
        ->>>>>>>> Module load ./entry-writer.js: 37
        ->>>>>>> Module load ./lib/pack.js: 39
        ->>>>>> Module load tar: 44
        ->>>>> Module load ../../util/unpack: 46
        ->>>> Module load ../plugman/registry/registry: 49
        ->>> Module load ./plugin: 54
        ->> Module load ./restore-util: 54
        ->>>>>> Module load acorn: 6
        ->>>>> Module load falafel: 7
        ->>>>> Module load ./loadConfig: 3
        ->>>> Module load browserify-transform-tools: 13
        ->>> Module load aliasify: 15
        ->>>>>>> Module load resolve: 3
        ->>>>>> Module load browser-resolve: 4
        ->>>>>>> Module load acorn: 20
        ->>>>>>>> Module load estraverse: 3
        ->>>>>>>> Module load esutils: 4
        ->>>>>>> Module load escodegen: 11
        ->>>>>> Module load detective: 33
        ->>>>>>>>>> Module load ./_stream_readable: 3
        ->>>>>>>>> Module load ./_stream_duplex: 9
        ->>>>>>>> Module load ./lib/_stream_transform.js: 9
        ->>>>>>> Module load readable-stream/transform: 10
        ->>>>>> Module load through2: 12
        ->>>>>> Module load concat-stream: 3
        ->>>>>>>>>>> Module load ./_stream_readable: 3
        ->>>>>>>>>> Module load ./_stream_duplex: 7
        ->>>>>>>>> Module load ./lib/_stream_transform.js: 7
        ->>>>>>>> Module load readable-stream/transform: 8
        ->>>>>>> Module load through2: 8
        ->>>>>> Module load stream-combiner2: 11
        ->>>>> Module load module-deps: 70
        ->>>>> Module load deps-sort: 3
        ->>>>>> Module load JSONStream: 3
        ->>>>>>>>>> Module load ./_stream_readable: 3
        ->>>>>>>>> Module load ./_stream_duplex: 6
        ->>>>>>>> Module load ./lib/_stream_transform.js: 7
        ->>>>>>> Module load readable-stream/transform: 8
        ->>>>>> Module load through2: 8
        ->>>>>>>>> Module load ./source-map/source-map-generator: 10
        ->>>>>>>>> Module load ./source-map/source-node: 3
        ->>>>>>>> Module load source-map: 14
        ->>>>>>> Module load inline-source-map: 15
        ->>>>>>>>> Module load ./source-map/source-map-generator: 9
        ->>>>>>>>> Module load ./source-map/source-map-consumer: 3
        ->>>>>>>> Module load source-map: 15
        ->>>>>>> Module load ./lib/mappings-from-map: 16
        ->>>>>> Module load combine-source-map: 33
        ->>>>> Module load browser-pack: 46
        ->>>>>>>> Module load acorn: 18
        ->>>>>>> Module load astw: 18
        ->>>>>> Module load lexical-scope: 19
        ->>>>>>>>> Module load ./source-map/source-map-generator: 8
        ->>>>>>>>> Module load ./source-map/source-map-consumer: 5
        ->>>>>>>> Module load source-map: 16
        ->>>>>>> Module load inline-source-map: 17
        ->>>>>> Module load combine-source-map: 21
        ->>>>> Module load insert-module-globals: 43
        ->>>>>> Module load acorn: 25
        ->>>>> Module load syntax-error: 26
        ->>>>> Module load ./lib/builtins.js: 9
        ->>>>>> Module load stream-splicer: 4
        ->>>>> Module load labeled-stream-splicer: 5
        ->>>> Module load browserify: 208
        ->>> Module load cordova-js/tasks/lib/bundle-browserify: 216
        ->> Module load ../plugman/browserify: 234
        -> Module load ./prepare: 319
        ->>>>>>>> Module load sax: 3
        ->>>>>>> Module load ./sax: 4
        ->>>>>> Module load ./parsers/index: 4
        ->>>>> Module load ./parser: 5
        ->>>> Module load elementtree: 9
        +>> Module load chalk: 12
        +>>> Module load semver: 3
        +>> Module load semver-diff: 4
        +> Module load update-notifier: 46
        +> Module load ./src/platforms/platforms: 4
        +>>>>>>> Module load underscore: 4
        +>>>>>>>>>>> Module load sax: 3
        +>>>>>>>>>> Module load ./sax: 3
        +>>>>>>>>> Module load ./parsers/index: 4
        +>>>>>>>> Module load ./parser: 4
        +>>>>>>> Module load elementtree: 8
        +>>>>>> Module load ../util/xml-helpers: 14
        +>>>>> Module load ./PluginInfo: 15
        +>>>> Module load ./src/PluginInfo/PluginInfoProvider.js: 16
        +>>> Module load ./scriptsFinder: 18
        +>>>> Module load q: 5
        +>>> Module load ./src/superspawn: 8
        +>> Module load ../hooks/HooksRunner: 29
        +> Module load ./build: 30
        +>> Module load ./src/PlatformJson: 5
        +>>> Module load shelljs: 6
        +>>> Module load ../plugman/pla`tforms/common: 3
        +>>> Module load ./src/ActionStack: 3
        +>>>> Module load semver: 5
        +>>> Module load ./src/ConfigChanges/ConfigChanges.js: 11
        +>> Module load ../platforms/PlatformApiPoly: 26
        +> Module load ./prepare: 36
        +>>>>>>>> Module load sax: 4
        +>>>>>>> Module load ./sax: 5
        +>>>>>> Module load ./parsers/index: 6
        +>>>>> Module load ./parser: 6
        +>>>> Module load elementtree: 12
        >>>>> Module load underscore: 5
        >>>> Module load ./src/util/xml-helpers: 7
        ->>> Module load ./AndroidManifest: 17
        +>>> Module load ./AndroidManifest: 21
        >>>> Module load shelljs: 6
        >>> Module load ./pluginHandlers: 8
        ->> Module load ./lib/AndroidProject: 28
        +>> Module load ./lib/AndroidProject: 31
        >>> Module load q: 5
        ->>> Module load ./PlatformJson: 4
        +>>> Module load ./PlatformJson: 5
        +>>>> Module load semver: 3
        >>> Module load ./ConfigChanges/ConfigChanges: 5
        ->> Module load ./src/PluginManager: 19
        ->> Module load ./src/CordovaLogger: 3
        -> Module load /Users/nikhil/Developer/apps/perf/platforms/android/cordova/Api.js: 50
        -load: 764ms
        +>> Module load ./src/PluginManager: 20
        +> Module load /Users/nikhil/Developer/apps/perf/platforms/android/cordova/Api.js: 54
        +load: 341ms
        > Module load ./lib/prepare: 5
        ANDROID_HOME=/Users/nikhil/Library/Android/sdk
        JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home
        ->> Module load ./Adb: 3
        -> Module load ./lib/build: 7
        +> Module load ./lib/build: 6
        +> Module load ./GradleBuilder: 4
        :preBuild UP-TO-DATE
        :preDebugBuild UP-TO-DATE
        :checkDebugManifest
        @@ -171,6 +119,6 @@ JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home

        BUILD SUCCESSFUL

        -Total time: 1.027 secs
        +Total time: 1.244 secs
        Built the following apk(s):
        /Users/nikhil/Developer/apps/perf/platforms/android/build/outputs/apk/android-debug.apk
        ```

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

        $ git pull https://github.com/MSOpenTech/cordova-lib faster

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

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


        commit 1d5de898689886dfe71f47a07fa9a9a01cf65c4c
        Author: Nikhil Khandelwal <nikhilkh@microsoft.com>
        Date: 2016-05-03T16:10:25Z

        CB-11194 Improve cordova load time


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user nikhilkh opened a pull request: https://github.com/apache/cordova-lib/pull/434 CB-11194 Improve cordova load time I instrumented module load and this change improves our load time by ~50% (300+ ms) for `cordova build android` on a hello world project. Biggest gains are from delay loading only when required: browserify unorm unpack ```diff — a/Users/nikhil/Developer/cordova/require.log +++ b/Users/nikhil/Developer/cordova/require-after.log @@ -1,116 +1,64 @@ > Module load underscore: 4 ->>> Module load graceful-fs: 3 ->>> Module load osenv: 3 ->>> Module load write-file-atomic: 4 ->> Module load configstore: 17 ->>>> Module load color-convert: 4 +>> Module load child_process: 3 +>>> Module load graceful-fs: 6 +>>> Module load osenv: 4 +>>>>> Module load crypto: 4 +>>>> Module load ./rng: 5 +>>> Module load uuid: 6 +>>> Module load write-file-atomic: 3 +>> Module load configstore: 24 +>>>> Module load color-convert: 5 >>> Module load ansi-styles: 6 ->> Module load chalk: 11 ->> Module load semver-diff: 3 -> Module load update-notifier: 36 ->> Module load ../hooks/HooksRunner: 3 -> Module load ./build: 3 ->>> Module load unorm: 24 ->> Module load ../platforms/PlatformApiPoly: 30 ->>>>>>>>>>>> Module load graceful-fs: 12 ->>>>>>>>>>> Module load ./lib/reader.js: 13 ->>>>>>>>>>>>>> Module load minimatch: 3 ->>>>>>>>>>>>>> Module load inflight: 3 ->>>>>>>>>>>>> Module load glob: 9 ->>>>>>>>>>>> Module load rimraf: 10 ->>>>>>>>>>> Module load ./lib/writer.js: 15 ->>>>>>>>>> Module load fstream: 33 ->>>>>>>>> Module load ./entry.js: 35 ->>>>>>>> Module load ./entry-writer.js: 37 ->>>>>>> Module load ./lib/pack.js: 39 ->>>>>> Module load tar: 44 ->>>>> Module load ../../util/unpack: 46 ->>>> Module load ../plugman/registry/registry: 49 ->>> Module load ./plugin: 54 ->> Module load ./restore-util: 54 ->>>>>> Module load acorn: 6 ->>>>> Module load falafel: 7 ->>>>> Module load ./loadConfig: 3 ->>>> Module load browserify-transform-tools: 13 ->>> Module load aliasify: 15 ->>>>>>> Module load resolve: 3 ->>>>>> Module load browser-resolve: 4 ->>>>>>> Module load acorn: 20 ->>>>>>>> Module load estraverse: 3 ->>>>>>>> Module load esutils: 4 ->>>>>>> Module load escodegen: 11 ->>>>>> Module load detective: 33 ->>>>>>>>>> Module load ./_stream_readable: 3 ->>>>>>>>> Module load ./_stream_duplex: 9 ->>>>>>>> Module load ./lib/_stream_transform.js: 9 ->>>>>>> Module load readable-stream/transform: 10 ->>>>>> Module load through2: 12 ->>>>>> Module load concat-stream: 3 ->>>>>>>>>>> Module load ./_stream_readable: 3 ->>>>>>>>>> Module load ./_stream_duplex: 7 ->>>>>>>>> Module load ./lib/_stream_transform.js: 7 ->>>>>>>> Module load readable-stream/transform: 8 ->>>>>>> Module load through2: 8 ->>>>>> Module load stream-combiner2: 11 ->>>>> Module load module-deps: 70 ->>>>> Module load deps-sort: 3 ->>>>>> Module load JSONStream: 3 ->>>>>>>>>> Module load ./_stream_readable: 3 ->>>>>>>>> Module load ./_stream_duplex: 6 ->>>>>>>> Module load ./lib/_stream_transform.js: 7 ->>>>>>> Module load readable-stream/transform: 8 ->>>>>> Module load through2: 8 ->>>>>>>>> Module load ./source-map/source-map-generator: 10 ->>>>>>>>> Module load ./source-map/source-node: 3 ->>>>>>>> Module load source-map: 14 ->>>>>>> Module load inline-source-map: 15 ->>>>>>>>> Module load ./source-map/source-map-generator: 9 ->>>>>>>>> Module load ./source-map/source-map-consumer: 3 ->>>>>>>> Module load source-map: 15 ->>>>>>> Module load ./lib/mappings-from-map: 16 ->>>>>> Module load combine-source-map: 33 ->>>>> Module load browser-pack: 46 ->>>>>>>> Module load acorn: 18 ->>>>>>> Module load astw: 18 ->>>>>> Module load lexical-scope: 19 ->>>>>>>>> Module load ./source-map/source-map-generator: 8 ->>>>>>>>> Module load ./source-map/source-map-consumer: 5 ->>>>>>>> Module load source-map: 16 ->>>>>>> Module load inline-source-map: 17 ->>>>>> Module load combine-source-map: 21 ->>>>> Module load insert-module-globals: 43 ->>>>>> Module load acorn: 25 ->>>>> Module load syntax-error: 26 ->>>>> Module load ./lib/builtins.js: 9 ->>>>>> Module load stream-splicer: 4 ->>>>> Module load labeled-stream-splicer: 5 ->>>> Module load browserify: 208 ->>> Module load cordova-js/tasks/lib/bundle-browserify: 216 ->> Module load ../plugman/browserify: 234 -> Module load ./prepare: 319 ->>>>>>>> Module load sax: 3 ->>>>>>> Module load ./sax: 4 ->>>>>> Module load ./parsers/index: 4 ->>>>> Module load ./parser: 5 ->>>> Module load elementtree: 9 +>> Module load chalk: 12 +>>> Module load semver: 3 +>> Module load semver-diff: 4 +> Module load update-notifier: 46 +> Module load ./src/platforms/platforms: 4 +>>>>>>> Module load underscore: 4 +>>>>>>>>>>> Module load sax: 3 +>>>>>>>>>> Module load ./sax: 3 +>>>>>>>>> Module load ./parsers/index: 4 +>>>>>>>> Module load ./parser: 4 +>>>>>>> Module load elementtree: 8 +>>>>>> Module load ../util/xml-helpers: 14 +>>>>> Module load ./PluginInfo: 15 +>>>> Module load ./src/PluginInfo/PluginInfoProvider.js: 16 +>>> Module load ./scriptsFinder: 18 +>>>> Module load q: 5 +>>> Module load ./src/superspawn: 8 +>> Module load ../hooks/HooksRunner: 29 +> Module load ./build: 30 +>> Module load ./src/PlatformJson: 5 +>>> Module load shelljs: 6 +>>> Module load ../plugman/pla`tforms/common: 3 +>>> Module load ./src/ActionStack: 3 +>>>> Module load semver: 5 +>>> Module load ./src/ConfigChanges/ConfigChanges.js: 11 +>> Module load ../platforms/PlatformApiPoly: 26 +> Module load ./prepare: 36 +>>>>>>>> Module load sax: 4 +>>>>>>> Module load ./sax: 5 +>>>>>> Module load ./parsers/index: 6 +>>>>> Module load ./parser: 6 +>>>> Module load elementtree: 12 >>>>> Module load underscore: 5 >>>> Module load ./src/util/xml-helpers: 7 ->>> Module load ./AndroidManifest: 17 +>>> Module load ./AndroidManifest: 21 >>>> Module load shelljs: 6 >>> Module load ./pluginHandlers: 8 ->> Module load ./lib/AndroidProject: 28 +>> Module load ./lib/AndroidProject: 31 >>> Module load q: 5 ->>> Module load ./PlatformJson: 4 +>>> Module load ./PlatformJson: 5 +>>>> Module load semver: 3 >>> Module load ./ConfigChanges/ConfigChanges: 5 ->> Module load ./src/PluginManager: 19 ->> Module load ./src/CordovaLogger: 3 -> Module load /Users/nikhil/Developer/apps/perf/platforms/android/cordova/Api.js: 50 -load: 764ms +>> Module load ./src/PluginManager: 20 +> Module load /Users/nikhil/Developer/apps/perf/platforms/android/cordova/Api.js: 54 +load: 341ms > Module load ./lib/prepare: 5 ANDROID_HOME=/Users/nikhil/Library/Android/sdk JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home ->> Module load ./Adb: 3 -> Module load ./lib/build: 7 +> Module load ./lib/build: 6 +> Module load ./GradleBuilder: 4 :preBuild UP-TO-DATE :preDebugBuild UP-TO-DATE :checkDebugManifest @@ -171,6 +119,6 @@ JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.8.0_51.jdk/Contents/Home BUILD SUCCESSFUL -Total time: 1.027 secs +Total time: 1.244 secs Built the following apk(s): /Users/nikhil/Developer/apps/perf/platforms/android/build/outputs/apk/android-debug.apk ``` You can merge this pull request into a Git repository by running: $ git pull https://github.com/MSOpenTech/cordova-lib faster Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/434.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 #434 commit 1d5de898689886dfe71f47a07fa9a9a01cf65c4c Author: Nikhil Khandelwal <nikhilkh@microsoft.com> Date: 2016-05-03T16:10:25Z CB-11194 Improve cordova load time
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61912274

        — Diff: cordova-common/cordova-common.js —
        @@ -3,7 +3,7 @@
        or more contributor license agreements. See the NOTICE file
        distributed with this work for additional information
        regarding copyright ownership. The ASF licenses this file

        • to you under the Apache License, Version 2.0 (the
          + to you under the Apache License } Version 2.0 (the
            • End diff –

        fix this

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61912274 — Diff: cordova-common/cordova-common.js — @@ -3,7 +3,7 @@ or more contributor license agreements. See the NOTICE file distributed with this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the + to you under the Apache License } Version 2.0 (the End diff – fix this
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61912394

        — Diff: cordova-common/cordova-common.js —
        @@ -17,26 +17,40 @@
        under the License.
        */

        -exports = module.exports =

        { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -}

        ;
        +function addProperty(obj, property, modulePath) {
        + var val = null;
        +
        + // Add properties as getter to delay load the modules on first invocation
        + Object.defineProperty(obj, property, {
        + configurable: true,
        + get: function () {
        + val = val || require(modulePath);
        + ob[property] = val;
        — End diff –

        Typo, should be 'obj', not 'ob' ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61912394 — Diff: cordova-common/cordova-common.js — @@ -17,26 +17,40 @@ under the License. */ -exports = module.exports = { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -} ; +function addProperty(obj, property, modulePath) { + var val = null; + + // Add properties as getter to delay load the modules on first invocation + Object.defineProperty(obj, property, { + configurable: true, + get: function () { + val = val || require(modulePath); + ob [property] = val; — End diff – Typo, should be 'obj', not 'ob' ?
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61912923

        — Diff: cordova-common/cordova-common.js —
        @@ -17,26 +17,40 @@
        under the License.
        */

        -exports = module.exports =

        { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -}

        ;
        +function addProperty(obj, property, modulePath) {
        + var val = null;
        +
        + // Add properties as getter to delay load the modules on first invocation
        + Object.defineProperty(obj, property, {
        + configurable: true,
        + get: function () {
        + val = val || require(modulePath);
        + ob[property] = val;
        — End diff –

        Also, there should be no need to use the 'val' cache if you are overwriting obj[property].

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61912923 — Diff: cordova-common/cordova-common.js — @@ -17,26 +17,40 @@ under the License. */ -exports = module.exports = { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -} ; +function addProperty(obj, property, modulePath) { + var val = null; + + // Add properties as getter to delay load the modules on first invocation + Object.defineProperty(obj, property, { + configurable: true, + get: function () { + val = val || require(modulePath); + ob [property] = val; — End diff – Also, there should be no need to use the 'val' cache if you are overwriting obj [property] .
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61913141

        — Diff: cordova-common/cordova-common.js —
        @@ -17,26 +17,40 @@
        under the License.
        */

        -exports = module.exports =

        { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -}

        ;
        +function addProperty(obj, property, modulePath) {
        — End diff –

        Name this something more specific than addProperty, because it only adds a specific kind of required-module property.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61913141 — Diff: cordova-common/cordova-common.js — @@ -17,26 +17,40 @@ under the License. */ -exports = module.exports = { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -} ; +function addProperty(obj, property, modulePath) { — End diff – Name this something more specific than addProperty, because it only adds a specific kind of required-module property.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61913318

        — Diff: cordova-common/src/ConfigChanges/ConfigFile.js —
        @@ -17,13 +17,30 @@
        var fs = require('fs');
        var path = require('path');

        -var bplist = require('bplist-parser');
        -var et = require('elementtree');
        -var glob = require('glob');
        -var plist = require('plist');
        +var modules = {};
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        Can this be defined once and then required from other sources?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61913318 — Diff: cordova-common/src/ConfigChanges/ConfigFile.js — @@ -17,13 +17,30 @@ var fs = require('fs'); var path = require('path'); -var bplist = require('bplist-parser'); -var et = require('elementtree'); -var glob = require('glob'); -var plist = require('plist'); +var modules = {}; + +function addProperty(obj, property, modulePath) { — End diff – Can this be defined once and then required from other sources?
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61913509

        — Diff: cordova-common/src/superspawn.js —
        @@ -22,7 +22,7 @@ var fs = require('fs');
        var path = require('path');
        var _ = require('underscore');
        var Q = require('q');
        -var shell = require('shelljs');
        +var which = require('shelljs/src/which');
        — End diff –

        It looks like this relies on shelljs internal layout that may change in a future version.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61913509 — Diff: cordova-common/src/superspawn.js — @@ -22,7 +22,7 @@ var fs = require('fs'); var path = require('path'); var _ = require('underscore'); var Q = require('q'); -var shell = require('shelljs'); +var which = require('shelljs/src/which'); — End diff – It looks like this relies on shelljs internal layout that may change in a future version.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61914375

        — Diff: cordova-common/src/ConfigChanges/ConfigFile.js —
        @@ -17,13 +17,30 @@
        var fs = require('fs');
        var path = require('path');

        -var bplist = require('bplist-parser');
        -var et = require('elementtree');
        -var glob = require('glob');
        -var plist = require('plist');
        +var modules = {};
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        Perhaps add an optional 4th parameter which is the name of a property on the required module.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61914375 — Diff: cordova-common/src/ConfigChanges/ConfigFile.js — @@ -17,13 +17,30 @@ var fs = require('fs'); var path = require('path'); -var bplist = require('bplist-parser'); -var et = require('elementtree'); -var glob = require('glob'); -var plist = require('plist'); +var modules = {}; + +function addProperty(obj, property, modulePath) { — End diff – Perhaps add an optional 4th parameter which is the name of a property on the required module.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61915398

        — Diff: cordova-lib/src/platforms/PlatformApiPoly.js —
        @@ -474,7 +473,12 @@ function getCreateArgs(destinationDir, projectConfig, options) {
        // CB-6992 it is necessary to normalize characters
        // because node and shell scripts handles unicode symbols differently
        // We need to normalize the name to NFD form since iOS uses NFD unicode form

        • args.push(platformName == 'ios' ? unorm.nfd(projectConfig.name()) : projectConfig.name());
          + var name = projectConfig.name();
          + if (platformName == 'ios') {
          + var unorm = require('unorm');
          + name = unorm.nfd(projectConfig.name());
            • End diff –

        `name = unorm.nfd(name);`

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61915398 — Diff: cordova-lib/src/platforms/PlatformApiPoly.js — @@ -474,7 +473,12 @@ function getCreateArgs(destinationDir, projectConfig, options) { // CB-6992 it is necessary to normalize characters // because node and shell scripts handles unicode symbols differently // We need to normalize the name to NFD form since iOS uses NFD unicode form args.push(platformName == 'ios' ? unorm.nfd(projectConfig.name()) : projectConfig.name()); + var name = projectConfig.name(); + if (platformName == 'ios') { + var unorm = require('unorm'); + name = unorm.nfd(projectConfig.name()); End diff – `name = unorm.nfd(name);`
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61915826

        — Diff: cordova-common/src/superspawn.js —
        @@ -22,7 +22,7 @@ var fs = require('fs');
        var path = require('path');
        var _ = require('underscore');
        var Q = require('q');
        -var shell = require('shelljs');
        +var which = require('shelljs/src/which');
        — End diff –

        Also, does this really save any time? Lots of sources require shelljs, so I'd expect it to be fully loaded somewhere else anyway.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61915826 — Diff: cordova-common/src/superspawn.js — @@ -22,7 +22,7 @@ var fs = require('fs'); var path = require('path'); var _ = require('underscore'); var Q = require('q'); -var shell = require('shelljs'); +var which = require('shelljs/src/which'); — End diff – Also, does this really save any time? Lots of sources require shelljs, so I'd expect it to be fully loaded somewhere else anyway.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61917144

        — Diff: cordova-common/src/superspawn.js —
        @@ -22,7 +22,7 @@ var fs = require('fs');
        var path = require('path');
        var _ = require('underscore');
        var Q = require('q');
        -var shell = require('shelljs');
        +var which = require('shelljs/src/which');
        — End diff –

        this eliminates multiple copies of `shelljs` - since every module gets it's own version. There are two versions of cordova-common loaded and hence two shelljs modules.

        If we do update the version of shelljs and this breaks - it would be very obvious to fix. I have a PR in the works to submit to shelljs to delay load on their side.

        Show
        githubbot ASF GitHub Bot added a comment - Github user nikhilkh commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61917144 — Diff: cordova-common/src/superspawn.js — @@ -22,7 +22,7 @@ var fs = require('fs'); var path = require('path'); var _ = require('underscore'); var Q = require('q'); -var shell = require('shelljs'); +var which = require('shelljs/src/which'); — End diff – this eliminates multiple copies of `shelljs` - since every module gets it's own version. There are two versions of cordova-common loaded and hence two shelljs modules. If we do update the version of shelljs and this breaks - it would be very obvious to fix. I have a PR in the works to submit to shelljs to delay load on their side.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61917593

        — Diff: cordova-common/cordova-common.js —
        @@ -17,26 +17,40 @@
        under the License.
        */

        -exports = module.exports =

        { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -}

        ;
        +function addProperty(obj, property, modulePath) {
        + var val = null;
        +
        + // Add properties as getter to delay load the modules on first invocation
        + Object.defineProperty(obj, property, {
        + configurable: true,
        + get: function () {
        + val = val || require(modulePath);
        + ob[property] = val;
        — End diff –

        This was a last minute change - fixed already - yes, I can remove 'val'

        Show
        githubbot ASF GitHub Bot added a comment - Github user nikhilkh commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61917593 — Diff: cordova-common/cordova-common.js — @@ -17,26 +17,40 @@ under the License. */ -exports = module.exports = { - events: require('./src/events'), - superspawn: require('./src/superspawn'), - - ActionStack: require('./src/ActionStack'), - CordovaError: require('./src/CordovaError/CordovaError'), - CordovaLogger: require('./src/CordovaLogger'), - CordovaExternalToolErrorContext: require('./src/CordovaError/CordovaExternalToolErrorContext'), - PlatformJson: require('./src/PlatformJson'), - ConfigParser: require('./src/ConfigParser/ConfigParser.js'), - - PluginInfo: require('./src/PluginInfo/PluginInfo.js'), - PluginInfoProvider: require('./src/PluginInfo/PluginInfoProvider.js'), - - PluginManager: require('./src/PluginManager'), - - ConfigChanges: require('./src/ConfigChanges/ConfigChanges.js'), - ConfigKeeper: require('./src/ConfigChanges/ConfigKeeper.js'), - ConfigFile: require('./src/ConfigChanges/ConfigFile.js'), - mungeUtil: require('./src/ConfigChanges/munge-util.js'), - - xmlHelpers: require('./src/util/xml-helpers') -} ; +function addProperty(obj, property, modulePath) { + var val = null; + + // Add properties as getter to delay load the modules on first invocation + Object.defineProperty(obj, property, { + configurable: true, + get: function () { + val = val || require(modulePath); + ob [property] = val; — End diff – This was a last minute change - fixed already - yes, I can remove 'val'
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r61922956

        — Diff: cordova-common/src/ConfigChanges/ConfigFile.js —
        @@ -17,13 +17,30 @@
        var fs = require('fs');
        var path = require('path');

        -var bplist = require('bplist-parser');
        -var et = require('elementtree');
        -var glob = require('glob');
        -var plist = require('plist');
        +var modules = {};
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        +1

        Show
        githubbot ASF GitHub Bot added a comment - Github user rakatyal commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r61922956 — Diff: cordova-common/src/ConfigChanges/ConfigFile.js — @@ -17,13 +17,30 @@ var fs = require('fs'); var path = require('path'); -var bplist = require('bplist-parser'); -var et = require('elementtree'); -var glob = require('glob'); -var plist = require('plist'); +var modules = {}; + +function addProperty(obj, property, modulePath) { — End diff – +1
        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/434#discussion_r62007337

        — Diff: cordova-lib/cordova-lib.js —
        @@ -18,19 +18,35 @@
        */

        // For now expose plugman and cordova just as they were in the old repos
        +
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        Similar functionality already exists at [`src/cordova/util.js:339`](https://github.com/apache/cordova-lib/blob/master/cordova-lib/src/cordova/util.js#L339). There is a slight difference but it looks like it could be reused.

        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/434#discussion_r62007337 — Diff: cordova-lib/cordova-lib.js — @@ -18,19 +18,35 @@ */ // For now expose plugman and cordova just as they were in the old repos + + +function addProperty(obj, property, modulePath) { — End diff – Similar functionality already exists at [`src/cordova/util.js:339`] ( https://github.com/apache/cordova-lib/blob/master/cordova-lib/src/cordova/util.js#L339 ). There is a slight difference but it looks like it could be reused.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r62068625

        — Diff: cordova-lib/cordova-lib.js —
        @@ -18,19 +18,35 @@
        */

        // For now expose plugman and cordova just as they were in the old repos
        +
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        Nice, that should definitely be consolidated. It will have to be moved into cordova-common though.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r62068625 — Diff: cordova-lib/cordova-lib.js — @@ -18,19 +18,35 @@ */ // For now expose plugman and cordova just as they were in the old repos + + +function addProperty(obj, property, modulePath) { — End diff – Nice, that should definitely be consolidated. It will have to be moved into cordova-common though.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user codecov-io commented on the pull request:

        https://github.com/apache/cordova-lib/pull/434#issuecomment-216933920

          1. [Current coverage][cc-pull] is *80.71%*
            > Merging 434[cc-pull] into [master][cc-base-branch] will increase coverage by *+1.09%*

        ```diff
        @@ master #434 diff @@
        ==========================================
        Files 69 68 -1
        Lines 5399 5323 -76
        Methods 865 849 -16
        Messages 0 0
        Branches 1017 1011 -6
        ==========================================

        • Hits 4298 4296 -2
          + Misses 1101 1027 -74
          Partials 0 0
          ```

        1. File `...lugman/browserify.js` (not in diff) was deleted. [more](https://codecov.io/gh/apache/cordova-lib/commit/f4ec82b8cec151ad81962e10f8d4a223a1dcdaac/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E2F62726F777365726966792E6A73)

        > Powered by [Codecov](https://codecov.io?src=pr). Last updated by [cc83559...f4ec82b][cc-compare]
        [cc-base-branch]: https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr
        [cc-compare]: https://codecov.io/gh/apache/cordova-lib/compare/cc83559597db1a7367252c808bc05b697aea52db...f4ec82b8cec151ad81962e10f8d4a223a1dcdaac
        [cc-pull]: https://codecov.io/gh/apache/cordova-lib/pull/434?src=pr

        Show
        githubbot ASF GitHub Bot added a comment - Github user codecov-io commented on the pull request: https://github.com/apache/cordova-lib/pull/434#issuecomment-216933920 [Current coverage] [cc-pull] is * 80.71% * > Merging 434 [cc-pull] into [master] [cc-base-branch] will increase coverage by * +1.09% * ```diff @@ master #434 diff @@ ========================================== Files 69 68 -1 Lines 5399 5323 -76 Methods 865 849 -16 Messages 0 0 Branches 1017 1011 -6 ========================================== Hits 4298 4296 -2 + Misses 1101 1027 -74 Partials 0 0 ``` 1. File `...lugman/browserify.js` (not in diff) was deleted. [more] ( https://codecov.io/gh/apache/cordova-lib/commit/f4ec82b8cec151ad81962e10f8d4a223a1dcdaac/changes?src=pr#636F72646F76612D6C69622F7372632F706C75676D616E2F62726F777365726966792E6A73 ) > Powered by [Codecov] ( https://codecov.io?src=pr ). Last updated by [cc83559...f4ec82b] [cc-compare] [cc-base-branch] : https://codecov.io/gh/apache/cordova-lib/branch/master?src=pr [cc-compare] : https://codecov.io/gh/apache/cordova-lib/compare/cc83559597db1a7367252c808bc05b697aea52db...f4ec82b8cec151ad81962e10f8d4a223a1dcdaac [cc-pull] : https://codecov.io/gh/apache/cordova-lib/pull/434?src=pr
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r62247519

        — Diff: cordova-lib/cordova-lib.js —
        @@ -18,19 +18,35 @@
        */

        // For now expose plugman and cordova just as they were in the old repos
        +
        +
        +function addProperty(obj, property, modulePath) {
        — End diff –

        util.js has a side effect in global code of invoking a mkrdir. Also, the addModuleProperty is not quite the same and it's used in a number of places. I have already created a shared version in cordova-common.

        Show
        githubbot ASF GitHub Bot added a comment - Github user nikhilkh commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r62247519 — Diff: cordova-lib/cordova-lib.js — @@ -18,19 +18,35 @@ */ // For now expose plugman and cordova just as they were in the old repos + + +function addProperty(obj, property, modulePath) { — End diff – util.js has a side effect in global code of invoking a mkrdir. Also, the addModuleProperty is not quite the same and it's used in a number of places. I have already created a shared version in cordova-common.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user nikhilkh commented on the pull request:

        https://github.com/apache/cordova-lib/pull/434#issuecomment-217265794

        Looking to merge this by EOD today if there are no other concerns.

        Show
        githubbot ASF GitHub Bot added a comment - Github user nikhilkh commented on the pull request: https://github.com/apache/cordova-lib/pull/434#issuecomment-217265794 Looking to merge this by EOD today if there are no other concerns.
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/cordova-lib/pull/434#discussion_r62252220

        — Diff: cordova-common/src/superspawn.js —
        @@ -22,20 +22,20 @@ var fs = require('fs');
        var path = require('path');
        var _ = require('underscore');
        var Q = require('q');
        -var shell = require('shelljs');
        var events = require('./events');
        var iswin32 = process.platform == 'win32';

        // On Windows, spawn() for batch files requires absolute path & having the extension.
        function resolveWindowsExe(cmd) {
        + var which = require('shelljs/src/which');
        var winExtensions = ['.exe', '.bat', '.cmd', '.js', '.vbs'];
        function isValidExe(c)

        { return winExtensions.indexOf(path.extname(c)) !== -1 && fs.existsSync(c); }

        if (isValidExe(cmd))

        { return cmd; }
        • cmd = shell.which(cmd) || cmd;
          + cmd = which({}, cmd) || cmd;
          if (!isValidExe(cmd)) {
            • End diff –

        I think this won't work now. The non-wrapped `which()` returns a `ShellString` object, then `path.extname()` throws if `(typeof path !== 'string')`.
        Did you test on Windows?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jasongin commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/434#discussion_r62252220 — Diff: cordova-common/src/superspawn.js — @@ -22,20 +22,20 @@ var fs = require('fs'); var path = require('path'); var _ = require('underscore'); var Q = require('q'); -var shell = require('shelljs'); var events = require('./events'); var iswin32 = process.platform == 'win32'; // On Windows, spawn() for batch files requires absolute path & having the extension. function resolveWindowsExe(cmd) { + var which = require('shelljs/src/which'); var winExtensions = ['.exe', '.bat', '.cmd', '.js', '.vbs'] ; function isValidExe(c) { return winExtensions.indexOf(path.extname(c)) !== -1 && fs.existsSync(c); } if (isValidExe(cmd)) { return cmd; } cmd = shell.which(cmd) || cmd; + cmd = which({}, cmd) || cmd; if (!isValidExe(cmd)) { End diff – I think this won't work now. The non-wrapped `which()` returns a `ShellString` object, then `path.extname()` throws if `(typeof path !== 'string')`. Did you test on Windows?
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 441fd90e8a9e7e647ca3c19a796bbde01734cc50 in cordova-lib's branch refs/heads/master from Nikhil Khandelwal
        [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=441fd90 ]

        CB-11194 Improve cordova load time

        This closes #434

        Show
        jira-bot ASF subversion and git services added a comment - Commit 441fd90e8a9e7e647ca3c19a796bbde01734cc50 in cordova-lib's branch refs/heads/master from Nikhil Khandelwal [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=441fd90 ] CB-11194 Improve cordova load time This closes #434
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/cordova-lib/pull/434
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 68af465ca7906c47bebfa9428124e83855dda6f6 in cordova-lib's branch refs/heads/master from Jesse MacFadyen
        [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=68af465 ]

        CB-11194 Defer creating of libDir folder until something actually requests it

        This closes #462

        Show
        jira-bot ASF subversion and git services added a comment - Commit 68af465ca7906c47bebfa9428124e83855dda6f6 in cordova-lib's branch refs/heads/master from Jesse MacFadyen [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=68af465 ] CB-11194 Defer creating of libDir folder until something actually requests it This closes #462
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 647852e5e48f779cc99c91b470d35b177644c342 in cordova-lib's branch refs/heads/6.3.x from Jesse MacFadyen
        [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=647852e ]

        CB-11194 Defer creating of libDir folder until something actually requests it

        This closes #462

        Show
        jira-bot ASF subversion and git services added a comment - Commit 647852e5e48f779cc99c91b470d35b177644c342 in cordova-lib's branch refs/heads/6.3.x from Jesse MacFadyen [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=647852e ] CB-11194 Defer creating of libDir folder until something actually requests it This closes #462
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 68af465ca7906c47bebfa9428124e83855dda6f6 in cordova-lib's branch refs/heads/common-1.4.x from Jesse MacFadyen
        [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=68af465 ]

        CB-11194 Defer creating of libDir folder until something actually requests it

        This closes #462

        Show
        jira-bot ASF subversion and git services added a comment - Commit 68af465ca7906c47bebfa9428124e83855dda6f6 in cordova-lib's branch refs/heads/common-1.4.x from Jesse MacFadyen [ https://git-wip-us.apache.org/repos/asf?p=cordova-lib.git;h=68af465 ] CB-11194 Defer creating of libDir folder until something actually requests it This closes #462

          People

          • Assignee:
            nikhilkh Nikhil Khandelwal
            Reporter:
            nikhilkh Nikhil Khandelwal
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development