Code cleanup proposal

[remcohaszing]

No description provided.

[macdonst]

@remcohaszing all good ideas. I believe there is already work on-going to move to ES6 Promises and drop Q.

[stevengill]

I agree! Time to dump jshint for ESLint!

Cordova-lib is due for a refactor. I really hate that addproperty code. Not sure why we went that way in the beginning. Maybe @purplecabbage knows?

[purplecabbage]

addProperty is there because we don't always want to load everything. For example, if you only wanted to call cordova version there is no need to load every attached module.
Typically when a command is run, it will load cordova, perform one operation, and exit ...

When our tests are run, unfortunately we have a much different case where lots of modules are loaded, then lots of methods are called. Switching to actual unit tests gets rid of this difference.

The prototype bullshit that you see is mostly un-needed, I would rather make it more functional with unit tests than apply OOP principles that aren't needed. A more clearly defined contract between cordova-lib and cli consumers of it would make this much cleaner.

I am a 'no' typically on most new shiny ES features. I am fine with 'const', and 'let', but would like to hold off on adding ticks, and arrow functions ( for a bit )

Q() being replaced with plain old Promise! I love it, I just removed Q from the cordova-browser!

ESLint sounds great!

[filmaj]

The prototype bullshit that you see is mostly un-needed, I would rather make it more functional with unit tests than apply OOP principles that aren't needed. A more clearly defined contract between cordova-lib and cli consumers of it would make this much cleaner.

+9001

addProperty is there because we don't always want to load everything. For example, if you only wanted to call cordova version there is no need to load every attached module.
Typically when a command is run, it will load cordova, perform one operation, and exit ...

For the record, the original PR that implemented the lazy loading is here: apache/cordova-lib#434. That PR description references a magical number claiming a 300ms load improvement. I'm wondering if 300ms is worth the obfuscation and difficulty in testing that is what I see as the downsides to that implementation.

My ideal best-case in terms of easy of testing would be modules that are objects with functions exposed on the object. Then spying/stubbing/mocking becomes dead simple, and I'm sure we can remove a bunch of the integration tests and re-implement as unit tests.

[purplecabbage]

Let's reverse that test, and see what the difference is with changes that have been committed since then. This should be quick to compare ...
If it is still just a < 500ms difference then let's just undo it ...

[filmaj]

K, I'll see what I can whip up today.

[filmaj]

First I will post results just looking at the changes to cordova-common. I will post cordova-lib results in a follow-up comment.

Latest master of cordova-common, which includes lazy loading, on my Macbook Pro 2016 running node 6 has the following (rough) results:

Just requiring the top-level cordova-common module (so not including anything, since none of the sub-properties are being accessed):

src/cordova-lib/cordova-common on master via ⬢ v6.10.1
➔ node
> s=new Date(); cc=require('./cordova-common');e=new Date();console.log('took ' + (e-s) + 'ms');
took 6ms

Next, requiring and accessing the ConfigFile sub-property (which itself has lazy-loaded sub-properties):

src/cordova-lib/cordova-common on master via ⬢ v6.10.1
➔ node
> s=new Date(); cc=require('./cordova-common').ConfigFile;e=new Date();console.log('took ' + (e-s) + 'ms');
took 7ms

Finally, requiring and accessing one of ConfigFile's lazy loaded sub-properties, breaking through two "layers" of lazy loading:

src/cordova-lib/cordova-common on master via ⬢ v6.10.1
➔ node
> s=new Date(); cc=require('./cordova-common').ConfigFile.bplist;e=new Date();console.log('took ' + (e-s) + 'ms');
took 8ms

I have a branch of cordova-common with lazy loading removed on my fork here.

Requiring the module, which should pull in everything, has the following results:

src/cordova-lib/cordova-common on common-no-lazy-load via ⬢ v6.10.1
➔ node
> s=new Date(); cc=require('./cordova-common');e=new Date();console.log('took ' + (e-s) + 'ms');
took 298ms

[filmaj]

Alright, and some comparisons now for the cordova-lib code. The code for removal of all lazy-loading from both cordova-common and cordova-lib is on my no-lazy-load branch of my fork.

First, latest master (includes lazy load) results, referencing just the top level module - very similar to cordova-common:

src/cordova-lib/cordova-lib on master via ⬢ v6.10.1
➔ node
> s=new Date(); cl=require('./cordova-lib');e=new Date();console.log('took ' + (e-s) + 'ms');
took 6ms

Accessing cordova-lib's lazy-loaded cordova sub-property:

src/cordova-lib/cordova-lib on master via ⬢ v6.10.1
➔ node
> s=new Date(); cl=require('./cordova-lib').cordova;e=new Date();console.log('took ' + (e-s) + 'ms');
took 86ms

And accessing the lazy-loaded plugman property:

src/cordova-lib/cordova-lib on master via ⬢ v6.10.1
➔ node
> s=new Date(); cl=require('./cordova-lib').plugman;e=new Date();console.log('took ' + (e-s) + 'ms');
took 23ms

And with lazy loading completely removed:

src/cordova-lib/cordova-lib on no-lazy-load via ⬢ v6.10.1
➔ node
> s=new Date(); cl=require('./cordova-lib').plugman;e=new Date();console.log('took ' + (e-s) + 'ms');
took 309ms

Would still need to massage my no-lazy-load branch a bit - there are some addProperty calls still littered under src/plugman/plugman.js, and its behaviour is a bit different than the other ones in cordova-common and cordova-lib.

So it looks like indeed, it is still a ~300ms load time without lazy loading, and in the best case you are saving a few ms less than that if we leave lazy loading in.

[stevengill]

I feel like you have proven we should revert addProperty bs :)

[stevengill]

Created an issue to replace jshint with eslint. https://issues.apache.org/jira/browse/CB-12895

[filmaj]

Should I file an issue for axing lazy-load?

[purplecabbage]

Just out of curiousity, did you time the difference in running the tests? `time npm run jasmine` @purplecabbage risingj.com

…

On Wed, Jun 7, 2017 at 11:53 AM, Fil Maj ***@***.***> wrote: Should I file an issue for axing lazy-load? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC0NvbEPMM7K9hgjX3_c-7ZSBU6yCX-ks5sBvG2gaJpZM4Nv5JH> .

[filmaj]

@purplecabbage in the process of doing those right now. Just updating my repos/forks since some of the sub-projects from cordova-lib were broken out.

Here's the comparison for cordova-common:

Master branch:

219 specs, 0 failures, 1 pending spec
Finished in 1.098 seconds

npm test  3.52s user 0.65s system 103% cpu 4.039 total

And my no-lazy-load branch on my fork:

219 specs, 0 failures, 1 pending spec
Finished in 1.066 seconds

npm test  3.59s user 0.70s system 99% cpu 4.290 total

Funny that the spec execution itself is slightly faster, but the overall execution of npm test is a wee bit slower.

Going to now set up a proper no-lazy-load branch for cordova-lib and post the same comparison shortly.

[filmaj]

Alright, and comparison for the latest cordova-lib follows.

Master:

$ time npm test
...
Ran 468 of 472 specs
468 specs, 1 failure, 2 pending specs
Finished in 429.283 seconds
...
npm test  61.09s user 34.91s system 22% cpu 7:14.43 total

And off my no-lazy-load branch on my fork:

Ran 408 of 412 specs
408 specs, 1 failure, 2 pending specs
Finished in 490.423 seconds
...
npm test  84.22s user 43.91s system 25% cpu 8:19.47 total

So it looks like this affects test run times as well. However, worth noting that by removing the lazy loading code, I am confident we can rewrite a lot of the integration tests as unit tests, making them much faster. So, I don't think the decision on whether to keep or remove lazy loading should be based on test times at this point in time.

[filmaj]

P.S. I would happily volunteer to work on reviewing the current test coverage of the I/O-heavy tests, seeing if we double-up on test coverage anywhere, and simplifying/migrating to faster unit tests.
Also worth having a discussion if keeping certain integration tests are still useful to cordova devs. I could see that being the case. If so, I would recommend splitting those tests out into their repo.

[stevengill]

We have already started moving integration tests into the integration-tests folder in cordova-lib

[filmaj]

I'd like to bring up an issue related to two points in the proposal: support for callback-as-last-argument async handling.

This is related to both "use ES6 promises" as well as "remove addProperty functions".

Currently, all of the main cordova object's properties/modules (except for serve) invoke addProperty with the opt_wrap option, meaning, we detect if a function is passed as a last argument to these modules, and if so, invoke the module's promise, wrap it in a done call and pass the callback as the error handler to done (see the specific code here).

Can someone point me to instances where this particular invocation method is used, either internally or externally? I'm wondering how useful providing this mechanism is. If we are OK with removing it, and changing the library API such that we require promises to be used, then we can remove addProperty altogether and simplify the code to match @remcohaszing's proposal. If we cannot do that, then we still need to maintain this function-wrapping. I think. Maybe there are other options here - I'm all ears. I'd love to see a simple object-with-properties-as-module structure as described in this proposal be the standard. It would be so much easier to test!

[purplecabbage]

I'm pretty sure it's not used.
I say kill it and see who complains.

[filmaj]

Sweet. I'll update my branch later today.

Do you think we'd need to do a major version bump?

[filmaj]

Related to removal of the callback-as-last-argument wrapping, I believe we could then also remove the raw properties, as they would be the same as the top-level properties (both would simply point to the sub-modules directly).

[stevengill]

These sound like api changes to cordova-lib right? cordova.raw.* is how people currently interact with cordova-lib. We should be careful about breaking our API

[filmaj]

If we remove lazy loading and callback-wrapping-for-promise-compatibility, then there would be now difference between cordova.raw.foo and cordova.foo, making cordova.raw useless.

I am in favour of doing that removal and bumping the major version.

[filmaj]

I mean, I suppose for API compatibility we could leave the raw properties around and deprecate if we don't want to do a major version bump, but I do think that removing the callback-wrapping itself would be an API change and would warrant a major version bump, right?

[stevengill]

Wouldn't every library that relies on cordova-lib break? I'm against the removal without a proper deprecation notice. It would be better to make cordova.raw an alias if there is no difference between the two.

[purplecabbage]

We could deprecate the .raw interfaces, and just leave them pointed to the same things. Our API is NOT published, which to me is another huge issue, as we have no idea what little tricks others are using to pull functionality out of us. We should solidify the contract for anyone who wants to use cordova-lib in their projects, my expectation is that other clis probably start by looking at what and how cordova-cli does things, but over time they may browse the lib code and hook their dependencies deeper.

[filmaj]

OK I'll leave the raw properties as aliases to the top-level methods, and have them emit a warn event.

I will, however, update all internal code + tests to stop using them.

[filmaj]

My fork+branch is updated: https://github.com/filmaj/cordova-lib/tree/no-lazy-load
Still tweaking it as for some reason, removing use of raw internally makes four tests fail 🤔.

Here's what it would look like in practice:

~/src/cordova-lib on no-lazy-load [?]
➔ node
> c=require('./src/cordova/cordova')
{ binname: [Getter/Setter],
  on: [Function: on],
  off: [Function: off],
  removeListener: [Function: off],
  removeAllListeners: [Function: removeAllListeners],
  emit: [Function: emit],
  trigger: [Function: emit],
  findProjectRoot: [Function: findProjectRoot],
  prepare: { [Function: prepare] preparePlatforms: [Function: preparePlatforms] },
  build: [Function: build],
  config:
   { [Function: config]
     getAutoPersist: [Function],
     setAutoPersist: [Function],
     read: [Function: get_config],
     write: [Function: set_config],
     has_custom_path: [Function] },
  create: [Function],
  emulate: [Function: emulate],
  plugin: { [Function: plugin] getFetchVersion: [Function: getFetchVersion] },
  plugins: { [Function: plugin] getFetchVersion: [Function: getFetchVersion] },
  serve: [Function: server],
  platform:
   { [Function: platform]
     add: [Function: add],
     remove: [Function: remove],
     update: [Function: update],
     check: [Function: check],
     list: [Function: list],
     save: [Function: save],
     getPlatformDetailsFromDir: [Function: getPlatformDetailsFromDir] },
  platforms:
   { [Function: platform]
     add: [Function: add],
     remove: [Function: remove],
     update: [Function: update],
     check: [Function: check],
     list: [Function: list],
     save: [Function: save],
     getPlatformDetailsFromDir: [Function: getPlatformDetailsFromDir] },
  compile: [Function: compile],
  run: [Function: run],
  info: [Function: info],
  targets: [Function: targets],
  requirements: [Function: check_reqs],
  projectMetadata:
   { getPlatforms: [Function: getPlatforms],
     getPlugins: [Function: getPlugins] },
  clean: [Function: clean],
  raw:
   { prepare: [Function],
     build: [Function],
     config: [Function],
     emulate: [Function],
     plugin: [Function],
     plugins: [Function],
     serve: [Function],
     platform: [Function],
     platforms: [Function],
     compile: [Function],
     run: [Function],
     info: [Function],
     targets: [Function],
     requirements: [Function],
     projectMetadata: [Function],
     clean: [Function] } }
> events=require('cordova-common').events;events.on('warn', function(msg) { console.log(msg); }); c.raw.platform('list').done(function(result) { console.log('Result!', result); }, function(err) { console.error('Error!', err); });
Use of `cordova.raw.*` methods is deprecated and `cordova.raw` will be removed in a future release. Please migrate to using the top-level `cordova.*` methods instead.
undefined
> Error! { CordovaError: Current working directory is not a Cordova-based project.
    at Object.cdProjectRoot (/Users/maj/src/cordova-lib/src/cordova/util.js:152:15)
    at /Users/maj/src/cordova-lib/src/cordova/platform.js:656:40
    at _fulfilled (/Users/maj/src/cordova-lib/node_modules/q/q.js:787:54)
    at self.promiseDispatch.done (/Users/maj/src/cordova-lib/node_modules/q/q.js:816:30)
    at Promise.promise.promiseDispatch (/Users/maj/src/cordova-lib/node_modules/q/q.js:749:13)
    at /Users/maj/src/cordova-lib/node_modules/q/q.js:810:14
    at flush (/Users/maj/src/cordova-lib/node_modules/q/q.js:108:17)
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)
  name: 'CordovaError',
  message: 'Current working directory is not a Cordova-based project.',
  code: 0,
  context: undefined }

Note that the deprecation message will change based on which module's raw methods you are using: in cordova, it will say "cordova.raw will be removed in a future release", in plugman, it will say "plugman.raw will be removed in a future release", etc. The functionality for this I encapsulated in a separate alias module: https://github.com/filmaj/cordova-lib/blob/no-lazy-load/src/util/alias.js

[purplecabbage]

looking good!
I would prefer the file be named alias-deprecation.js as alias is only part of the story.

[filmaj]

I've issued a pull request that addresses at least the 'no lazy loading' part of this proposal (among other things) here in apache/cordova-lib#562