feat: expose logger API #161

OrenMe · 2017-11-11T18:21:19Z

Description of the Changes

set to ERROR level by default
expose API to set and get log level and get log level enums
expose convenience method to set logger level via global window param window.PLAYKIT_LOG_LEVEL
update to JS-Logger V1.4

CheckLists

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

set to ERROR level by default expose API to set and get log level and get log level enums expose convenience method to set logger level via global window param window.PLAYKIT_LOG_LEVEL update to JS-Logger V1.4

dan-ziv · 2017-11-12T07:43:56Z

src/utils/logger.js

-export default lf;
-export {LOG_LEVEL};
+export default getLogger;
+export {LOG_LEVEL, getLogLevel, setLogLevel};


change the var name LOG_LEVEL to LogLevel since it's an enum and then we can export it regularly without use the as syntax of flow in other files

@dan-ziv it's either an enum or not.
If it's an enum we should keep the convention as we do with other enums, like:
import {PLAYER_EVENTS as PlayerEvents, HTML5_EVENTS as Html5Events, CUSTOM_EVENTS as CustomEvents} from './event/events'

Those conventions are wrong and already fixed in my player typing PR which you all ignore of reviewing :)
From what I see in all the lead projects the correct conventions for enums should be lead with capitals (i.e. MyEnum) and constants should be all written in capital letters (i.e. MY_CONST)

dan-ziv · 2017-11-12T07:52:17Z

sug.txt

@@ -0,0 +1,107 @@
+/Users/OrenMe/repos/playkit-js/src/player.js


what this file for?

dan-ziv · 2017-11-12T07:53:06Z

text.md

+// create a player
+var player = KalturaPlayer.setup("player-placeholder", config);
+
+var textplayer.getTracks(player.Track.TEXT);


did u mean
var textTracks = player.getTracks(player.Track.TEXT);
?

dan-ziv · 2017-11-12T07:53:32Z

text.md

+var player = KalturaPlayer.setup("player-placeholder", config);
+
+var textplayer.getTracks(player.Track.TEXT);
+player.selectTrack(track)


where is track?

dan-ziv · 2017-11-12T07:56:11Z

src/player.js

@@ -322,6 +322,9 @@ export default class Player extends FakeEventTarget {
    this._createPlayerContainer();
    this._appendPosterEl();
    this.configure(config);
+    if (config.logLevel && this.LogLevel[config.logLevel]){


Why can't we do this in the loadPlayer factory function of playkit.js before the initialization of the player?
in configure we already write logs and if we are doing it after it won't affect this, no?

I see now that we are using this for the check, but we can define fallback level in such cases and avoid this

If you refer to the version and name printing in index.js, we don't want to print them and in any case these printouts are in load time before CTOR is called, and setting the config level can be done only in init time

No, I'm not referring to the version and name printouts.
I'm mean that you are setting the log level too late - after the configure method.
The configure method flow will print logs and seems like those logs won't have the correct log level.
Don't you think the correct place to handle the log level is before creating the player? in the loadPlayer method?

No, the static API should remain simple IMHO.
Constructor is where everything should be setup, and in the case of logs, this line should just some before others if it's more important.
Fixed by moving to be first in line

dan-ziv · 2017-11-12T07:57:35Z

@OrenMe
What are
cov.json
and
sug.txt
?

This reverts commit 805168c.

dan-ziv · 2017-11-12T16:00:06Z

src/player.js

@@ -322,6 +322,9 @@ export default class Player extends FakeEventTarget {
    this._createPlayerContainer();
    this._appendPosterEl();
    this.configure(config);
+    if (config.logLevel && this.LogLevel[config.logLevel]){


No, I'm not referring to the version and name printouts.
I'm mean that you are setting the log level too late - after the configure method.
The configure method flow will print logs and seems like those logs won't have the correct log level.
Don't you think the correct place to handle the log level is before creating the player? in the loadPlayer method?

dan-ziv · 2017-11-12T16:01:04Z

src/utils/logger.js

-export default lf;
-export {LOG_LEVEL};
+export default getLogger;
+export {LOG_LEVEL, getLogLevel, setLogLevel};


Those conventions are wrong and already fixed in my player typing PR which you all ignore of reviewing :)
From what I see in all the lead projects the correct conventions for enums should be lead with capitals (i.e. MyEnum) and constants should be all written in capital letters (i.e. MY_CONST)

dan-ziv · 2017-11-13T14:24:35Z

test/src/player.spec.js

@@ -2672,3 +2671,23 @@ describe('_resetTextCuesAndReposition', function () {
  });
 });

+describe('logger', ()=>{


prettify the code in this file

dan-ziv · 2017-11-13T14:25:12Z

src/playkit.js

@@ -21,8 +21,8 @@ declare var __VERSION__: string;
 declare var __NAME__: string;
 declare var __PACKAGE_URL__: string;

-LoggerFactory.getLogger().log(`%c ${__NAME__} ${__VERSION__}`, "color: #98ff98;  font-size: large");
-LoggerFactory.getLogger().log(`%c For more details see ${__PACKAGE_URL__}`, "color: #98ff98;");
+getLogger().log(`%c ${__NAME__} ${__VERSION__}`, "color: #98ff98;  font-size: large");


Lets delete those logs... ?

dan-ziv · 2017-11-13T14:27:21Z

src/player.js

@@ -306,6 +306,9 @@ export default class Player extends FakeEventTarget {
   */
  constructor(config: Object = {}) {
    super();
+    if (config.logLevel && LogLevel[config.logLevel]){


…into loggerRefactor

dan-ziv · 2017-11-14T06:27:05Z

@OrenMe approved just fix codeclimate and travis (same issue)

OrenMe added 4 commits November 11, 2017 19:43

feat: expose logger API

4c395e3

set to ERROR level by default expose API to set and get log level and get log level enums expose convenience method to set logger level via global window param window.PLAYKIT_LOG_LEVEL update to JS-Logger V1.4

add logger test

bbba944

change API to get and set the logLevel object

fcd17e5

fix tests

4f1e076

OrenMe self-assigned this Nov 11, 2017

OrenMe requested review from yairans, dan-ziv and odedhutzler November 11, 2017 18:21

OrenMe changed the title ~~Logger refactor~~ feat: expose logger API Nov 11, 2017

OrenMe added 4 commits November 11, 2017 20:27

remove debugger

7cb1b59

remove window.PLAYKIT_LOG_LEVEL

2cebb88

verify log level before setting on init

4fe7843

fix test

805168c

dan-ziv suggested changes Nov 12, 2017

View reviewed changes

OrenMe added 2 commits November 12, 2017 10:59

Revert "fix test"

8b8b78a

This reverts commit 805168c.

fix tests

0aad2b3

OrenMe added the Type: Feature label Nov 12, 2017

dan-ziv suggested changes Nov 12, 2017

View reviewed changes

OrenMe added 3 commits November 12, 2017 18:15

change enum notation

5b57e03

config log level earlier in CTOR

15b6c36

fix test

a45b693

dan-ziv suggested changes Nov 13, 2017

View reviewed changes

Dan Ziv and others added 5 commits November 13, 2017 18:12

Merge branch 'master' into loggerRefactor

964ac5b

remove redundent logs

92657bf

enable setting config from configure API on-the-fly

652d52f

prettify code

de018ce

Merge branch 'loggerRefactor' of https://github.com/kaltura/playkit-js …

b6c5995

…into loggerRefactor

dan-ziv approved these changes Nov 14, 2017

View reviewed changes

OrenMe and others added 2 commits November 14, 2017 09:10

remove unused import

ede6eb6

beautify

8d527ca

dan-ziv merged commit f61dce4 into master Nov 14, 2017

dan-ziv deleted the loggerRefactor branch November 14, 2017 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose logger API #161

feat: expose logger API #161

OrenMe commented Nov 11, 2017 •

edited

Loading

dan-ziv Nov 12, 2017

OrenMe Nov 12, 2017

dan-ziv Nov 12, 2017

OrenMe Nov 12, 2017

dan-ziv Nov 12, 2017

dan-ziv Nov 12, 2017

dan-ziv Nov 12, 2017

dan-ziv Nov 12, 2017

dan-ziv Nov 12, 2017 •

edited

Loading

OrenMe Nov 12, 2017

dan-ziv Nov 12, 2017 •

edited

Loading

OrenMe Nov 12, 2017

dan-ziv commented Nov 12, 2017

dan-ziv Nov 12, 2017 •

edited

Loading

dan-ziv Nov 12, 2017

dan-ziv Nov 13, 2017

dan-ziv Nov 13, 2017

dan-ziv Nov 13, 2017

dan-ziv commented Nov 14, 2017

		@@ -0,0 +1,107 @@
		/Users/OrenMe/repos/playkit-js/src/player.js

feat: expose logger API #161

feat: expose logger API #161

Conversation

OrenMe commented Nov 11, 2017 • edited Loading

Description of the Changes

CheckLists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv commented Nov 12, 2017

dan-ziv Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv commented Nov 14, 2017

OrenMe commented Nov 11, 2017 •

edited

Loading

dan-ziv Nov 12, 2017 •

edited

Loading

dan-ziv Nov 12, 2017 •

edited

Loading

dan-ziv Nov 12, 2017 •

edited

Loading