New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor CordovaLogger to singleton class #53

Merged
merged 1 commit into from Dec 18, 2018

Conversation

4 participants
@dpogue
Copy link
Member

dpogue commented Nov 28, 2018

What does this PR do?

Based on the discussion at apache/cordova-lib#721, this attempts to make CordovaLogger a proper singleton, such that we cannot end up with multiple instances of the logger even when multiple versions of cordova-common are in us (of course, that only applies if all those multiple versions are using this singleton pattern)

What testing has been done on this change?

All existing CordovaLogger tests pass locally.

It would be great to do a test with multiple instances to prove that this singleton pattern works, but I'm not even sure how we'd start setting up a test like that...

@dpogue dpogue requested a review from brodybits Nov 28, 2018

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Nov 28, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #53 into master will increase coverage by 0.03%.
The diff coverage is 97.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   88.85%   88.88%   +0.03%     
==========================================
  Files          19       20       +1     
  Lines        1795     1800       +5     
  Branches      368      368              
==========================================
+ Hits         1595     1600       +5     
  Misses        200      200
Impacted Files Coverage Δ
src/util/formatError.js 92.3% <92.3%> (ø)
src/CordovaLogger.js 98.46% <98.43%> (+1.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2110597...b854657. Read the comment docs.

* @returns {CordovaLogger} Logger instance
*/
static get () {
if (Object.getOwnPropertySymbols(global).indexOf(INSTANCE_KEY) === -1) {

This comment has been minimized.

@brodybits

brodybits Nov 28, 2018

Contributor

It is not clear to me what global is or means. Can you please explain?

This comment has been minimized.

@dpogue

dpogue Nov 29, 2018

Member

It's the global object. In a web browser, it would be window:
https://nodejs.org/dist/latest-v10.x/docs/api/globals.html#globals_global

This comment has been minimized.

@brodybits

brodybits Nov 29, 2018

Contributor

Got it. I would really appreciate pointers for some newer features such as global object & Symbol in the source or maybe in the commit message.

This comment has been minimized.

@dpogue

dpogue Nov 29, 2018

Member

global is a NodeJS specific thing, which I think has existed since the very early days.
Symbol is a built-in JS (ES6) language feature, which has been around for several years now.

I don't really think we should have comments everywhere providing an introduction to built-in language features, but I think it is probably valuable to have a link to https://derickbailey.com/2016/03/09/creating-a-true-singleton-in-node-js-with-es6-symbols/ somewhere to explain the singleton design pattern we're implementing. I'll amend the commit with that added.

This comment has been minimized.

@brodybits

brodybits Nov 29, 2018

Contributor

link to https://derickbailey.com/2016/03/09/creating-a-true-singleton-in-node-js-with-es6-symbols/

the link explains it all

I guess it should be good enough to put it in the commit message; personally I would have put it in the source code for the sake of absolute clarity.

this.addLevel('results', 10000);

this.setLevel('normal');
function formatError (error, isVerbose) {

This comment has been minimized.

@brodybits

brodybits Nov 28, 2018

Contributor

I am not so happy that the internal formatError utility function was moved for multiple reasons:

  • makes it much harder to understand what changed in the diff
  • I think higher-level API items belong up top and internal utility functions belong toward the end of the module.

This comment has been minimized.

@dpogue

dpogue Nov 29, 2018

Member

Yeah, that's a valid concern.

Personally I always find it non-obvious when functions are used in code before they are defined in the source. I know that's how JavaScript hoisting works, but I find it adds a bunch of mental overhead of keeping track of what's required from node modules and what's defined in the file but below where it's used.

This comment has been minimized.

@brodybits

brodybits Nov 29, 2018

Contributor

Argh

While I would not really accept that argument I don't think I can dismiss it as completely invalid.

What if we would factor formatError into an internal utility module?

Or look for an existing npm module that can do at least part of the job for us?

A crazy idea could be to turn formatError into a member function, don't think ES6 class explicitly supports private methods though.

This comment has been minimized.

@dpogue

dpogue Nov 29, 2018

Member

I thought about turning it into a member function. You're right that it wouldn't really be private, but if we made it a utility then it wouldn't really be private either.

I don't have a strong preference for either option, and we already have a utils folder, so maybe extracting it to there is the best option?

This comment has been minimized.

@brodybits

brodybits Nov 29, 2018

Contributor

I would definitely favor extracting it into a module in the utils folder.

@brodybits
Copy link
Contributor

brodybits left a comment

Sorry this review has taken so long.

On the plus side I am very happy with the changes on a functional level.

My understanding is that there are 2 major parts of the change:

The bad part is that I find the actual changes pretty hard to follow, even if I do git show -b to ignore indentation changes.

In order to better understand the flow of the changes I made a logger-instance-in-smaller-commits branch on my fork that makes all of the changes except for moving formatError in smaller steps (with npm test succeeding on each of the commits):

(Disregard my @brodybits TBD ??? comment, thanks for explaining on GitHub.)

I will understand if the smaller commits that I proposed are not desired, would like the changes to be easier to understand somehow.

I would also appreciate some kind of resolution for the code comments I raised.

Apache Cordova: Tooling Pull Requests automation moved this from 🐣 New PR / Untriaged to 🙅 Pending Approval Nov 29, 2018

@dpogue dpogue force-pushed the dpogue:logger-instance branch from 42003fc to b854657 Nov 29, 2018

@dpogue

This comment has been minimized.

Copy link
Member

dpogue commented Nov 29, 2018

@brodybits Updated with the requested changes

@janpio janpio added the enhancement label Nov 29, 2018

@brodybits
Copy link
Contributor

brodybits left a comment

Much better with the separate module and comment with a link to the singleton instance pattern. I have a couple questions that remain:

  • Why make the functional singleton instance change and refactor to an ES6 class at the same time?
  • I forgot to ask yesterday: what is the motivation to refactor to an ES6 class in the first place?

I am still not a big fan of ES6 classes and just sent my opinion to dev@cordova.apache.org for discussion. I wish I would have thought of this before you started the work.

Apache Cordova: Tooling Pull Requests automation moved this from 🙅 Pending Approval to ✅ Approved, waiting for Merge Dec 2, 2018

@brodybits
Copy link
Contributor

brodybits left a comment

Let's get this one merged. Much better than what we had before.

I continue to think we should be able to accomplish the same thing with 1-2 object factory functions and would like to give it a try someday.

I think we have to at least increase the minor release version, maybe the major release version.

@dpogue

This comment has been minimized.

Copy link
Member

dpogue commented Dec 3, 2018

I think we have to at least increase the minor release version, maybe the major release version.

I don't think this actually requires anything more than a patch release, since the exposed API hasn't changed.

One thing to note is that we will have to make a similar change to CordovaEvents (unrelated to the change here, but needed for the same reason) and that one might need to be a breaking change

@brodybits brodybits referenced this pull request Dec 18, 2018

Open

Cordova 9 release plan #10

7 of 32 tasks complete

@brodybits brodybits merged commit 6667cfc into apache:master Dec 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment