Initial contribution core/log.js #268

Martin-Stangl · 2019-10-18T05:16:12Z

Integration of my logger as a core library.

Fully backwards compatible: Logging functions in core/utils.js have been kept but were adjusted to use the new core/log.js.

Documentation is not done yet. The sun is already rising and I want some sleep :-).
I will add it, once the implementation is considered good to merge.
But it works pretty much as described here: https://github.com/Martin-Stangl/openHAB-javascript-rule-libraries/blob/master/README.md. The relevant differences are:

Log line formats changed slightly.
Notification levels are now NOTIFY_OFF, NOTIFY_ERROR, NOTIFY_WARN, NOTIFY_INFO and NOTIFY_DEBUG. (I was worried to create a conflict if I throw a variable called OFF into the global context. Not an issue anymore, when we adopt a module architecture.)
Support for parameterized logging added: logger.debug("The new entry is {}. It replaces {}.", entry, oldEntry);

Let me know if this needs any major changes or if I can go ahead an prepare the documentation.
Thanks.

Martin-Stangl · 2019-10-18T20:34:58Z

After getting a little bit of sleep, I think I want to make the compatibility functionality, which was the last thing I added in, the default behavior. This means getting rid of the need to wrap the message in an Error object.

Also I noticed that the console.log function in utils.js just creates an INFO log entry with the text „console.log“. I initially kept the behavior in the rewrite, but it would make more sense to fix it.So I will do that.

…nsole log functions improved/fixed

Martin-Stangl · 2019-10-19T02:35:52Z

I completed the changes:

It is now possible to log an Error object or a string: log.error("Test") and log.error(Error("Test")) produce the same result.
Parameterized notifications fixed. I basically forgot to implement it initially, when I did it for the logging part.
context.console.log fixed and console logs now log to [jsr223.javascript.console]
Minor changes to the "short" log format.

This should be it (except for the missing documentation).

PS: Next step would then be probably to replace all core logging calls to use log.js. It is not mandatory, as the utils.js log functions work as before (output format is slightly different), but I saw that a lot of error logging uses LINE to specify the error position instead of err.lineNumber. This makes the error messages point to the line where the error is logged instead of the actual line, where the error occurs, which is not ideal.

5iver · 2019-10-19T17:19:19Z

All of the logging in the JS libraries should go through this new lib. The other logging functions should be removed. I will review as soon as I can find some time. Hopefully @lewie will go through this too!

@jpg0, you had a library for logging in your test commit. Does the functionality in this PR align with yours? Do you have any suggestions or improvements?

The JS libraries are still under construction, so do what you can, but don't get blocked if you can't keep backwards compatibility.

Martin-Stangl · 2019-10-22T06:59:31Z

Another remark: One thing utils.js does right now is to override console.log and the other console.* logging functions. I did not move this to log.js, as I am not sure it is needed, but kept in in utils.js instead.

My thought also is that at some point the backwards compatibility support could be dropped by just cleaning up utils.js. Or we could meanwhile collect these things in a separate compatibility library, so we can also clean up utils.js earlier.

jpg0 · 2019-10-22T09:22:07Z

Just got this from your library:
2019-10-22 20:18:44.503 [ERROR] [jsr223.javascript.schedules ] - [source /etc/openhab2/automation/lib/javascript/personal/log.js, line 121, function _getLogMessage] Cannot read property "split" from undefined

I expect that I fed it a bad (error) object to log or something, but that error message is pretty cryptic and looks like it hit an edge-case. (The error I fed it was: java.lang.ClassCastException: Cannot cast jdk.nashorn.internal.runtime.Undefined to java.util.Collection)

Martin-Stangl · 2019-10-22T11:01:36Z

Just got this from your library:
2019-10-22 20:18:44.503 [ERROR] [jsr223.javascript.schedules ] - [source /etc/openhab2/automation/lib/javascript/personal/log.js, line 121, function _getLogMessage] Cannot read property "split" from undefined

I expect that I fed it a bad (error) object to log or something, but that error message is pretty cryptic and looks like it hit an edge-case. (The error I fed it was: java.lang.ClassCastException: Cannot cast jdk.nashorn.internal.runtime.Undefined to java.util.Collection)

Yes, this is caused by providing an object, which is not an Error object. I wanted to check, if I got an Error object or a child object of Error object, but knew this will be tricky. So I just made a "cheaper" implementation first but then forgot to revisit it.
I think it is not an even an edge-case, but something many people might do: Just temporary log out an object for debugging purposes, to figure out what is going on.
I will provide a fix.

5iver

It's great to see some other people working in the repo! Your technical JS support will best come from @lewie, but you'll find his time is even more limited than mine!

As a general statement, the preference is to use snake_case in the helper libraries for all languages. You'll see this in the newer JS libs. This helps to keep documentation consistent with Python and makes it easier to recognize when you are using Java. WDYT?

Core/automation/lib/javascript/core/log.js

5iver · 2019-10-22T17:40:29Z

Core/automation/lib/javascript/core/log.js

+                            case "logError":
+                            case "logWarn":
+                            case "logInfo":
+                            case "logDebug":
+                            case "logTrace":


I'm concerned about the use of the LogAction method names and potential conflicts. The utils.js lib did the same. My guess is that this was to provide a familiar name for people who had used the rules DSL. Now that actions.js is available and LogAction is easily accessible, this should be removed. This library should provide similar functionality to the Python core.log and provide a bridge between the native JS logging (console.log) and slf4j. Although, I have questioned the bridging to slf4j and have considered swapping it with LogAction.

This part was done specifically for support backwards compability support. If we get rid of logError, logWarn, ... in utils.log, this part goes away as well.

The new log functions are used like this:

load(__DIR__+'/log.js'); var log = Logger(); log.error("Message"); log.warn("Message"); log.info("Message"); log.debug("Message"); log.trace("Message");

utils.js does actually already provide this bridge between console.log and slf4j. I can move this part to log.js by simply instantiating a logger instance called console. var console = Logger();
Just need to add a few methods like log() and methods to change the logger name, log level and notification feature configuration, which currently can only be set at initialization.

But I also want to check, if we can so easily pretend to be console.log. Console.log also has a few tricks up it´s sleeve and I am not sure, if we can emulate all of that. But most I can remember is about color and font styles, which we just have to ignore / do not need to support.

I could convert log.js to use LogAction pretty easily instead of using slf4j directly, as LogAction does nothing else than wrap slf4j.
This means loosing the level trace, which is no big issue. But it would make log.js slower. All the compilation of data about the log call position is very slow (a few milliseconds - forever for a logger). Therefore I check using slf4j's isErrorEnabled() and the likes, if it even makes sense to do all of this. Unfortunately LogAction does not expose these methods. (Actually, I am guilty of the same and should expose these in log.js.)

If we just want to look and behave like we are using LogAction under the hood, we could simply call our logger "org.eclipse.smarthome.model.script.jsr223.javascript" and get rid of the level trace.
Not sure, if LogAction adds some fancy formatting, which we would need to emulate.

I keep you posted on the console.log front.
Give me your thoughts on LogAction. Maybe we should try a pull request there to get the isEnabled() methods.

But I also want to check, if we can so easily pretend to be console.log. Console.log also has a few tricks up it´s sleeve and I am not sure, if we can emulate all of that.

The big question is if there is any benefit to using this over LogAction 😄. If concole.log has some cool stuff in it, then that may be what tips the scale. I think it is great to equalize the functionality between the JS and Jython libraries, but I really question if we need this (or the Jython) library at all.

'use strict'; var OPENHAB_CONF = Java.type("java.lang.System").getenv("OPENHAB_CONF"); load(OPENHAB_CONF + '/automation/lib/javascript/core/actions.js'); LogAction.logWarn("Rules", "Test")

The Jython core.log provides a helpful decorator for getting a traceback, and it sends a notification too, but LogAction can do everything else.

A big benefit to slf4j is the parameterized logging and only calculating the value to log if the log entry fits the logger. It's nice to have some performance heavy logging in place that doesn't slow things down when not in use. I haven't found a way to make use of this in Jython. Is this the isErrorEnabled() (I'll need to look that up)? A PR for LogAction to enable this would be really nice!

As I mentioned elsewhere, I believe that there is benefit to having a console.log defined (so that existing JS code works), but what that forwards the actual log messages to I have no opinion on, and it could be either slf4j or LogAction with no impact to consumers.

I had a look at console. Browser and node.js implementation.
The node.js is probably, the one we would like to emulate, as we are also a backend implementation.
But looking at the node.js documetation at https://nodejs.org/api/console.html, I would say console is a different, much bigger beast. Already the message formatting syntax is completely different and then there are quite some methods to be implemented.

Therefore I do not think that what I built should pretend to be console. At least the formatting syntax would need to be supported, then it would be just about features not available, but not differences in functionality.

We could make quick progress when we canibalize the node.js implementation, but I am not sure, if I am up for that right now. Maybe.

Core/automation/lib/javascript/core/log.js

5iver · 2019-10-22T18:51:42Z

Core/automation/lib/javascript/core/utils.js

+
+	load(__DIR__+'/log.js');
+	var logger 					= Logger(null);
+	var loggerConsole			= Logger("console");


All of the logging functionality should be stripped out of utils.js. The utils.js library will end up being used to hold helper functions for use in scripts and libs. This is a breaking change, but these should be expected... https://github.com/openhab-scripters/openhab-helper-libraries/blob/master/README.md#-under-construction-.

OK. I will the go ahead and remove this from utils.js.
But this means I have to rework all log calls all exiting core libraries.

So I first want to resolve, if we will emulate console.log (most likely yes).

Also it might be better that @jpg0 gets his CommonJS stuff done and merged into the master, before I start also changing all libraries.

But this means I have to rework all log calls all exiting core libraries.

I'm missing something... I did not think there was much effort to pull logging out of utils.js. But yes... if the core libs need reworked for logging, then it should wait for CommonJS, if that is the direction we're going... haven't dug into that one yet!

Well, I could just take the log methods as they are in utils.js and move them to log.js. But if we completely forego backwards compatibility and get rid of logError, logWarn, ..., I do not see much value in that. Better to leave them in utils.js, until we actually get rid of them and not pollute the new library with it.
And logError, logWarn, ...are used by the core libraries, So this needs to be changed to get rid of these methods.

Signed-off-by: Martin Stangl <[email protected]>

…-helper-libraries

Martin-Stangl · 2019-10-22T20:28:13Z

Just got this from your library:
2019-10-22 20:18:44.503 [ERROR] [jsr223.javascript.schedules ] - [source /etc/openhab2/automation/lib/javascript/personal/log.js, line 121, function _getLogMessage] Cannot read property "split" from undefined
I expect that I fed it a bad (error) object to log or something, but that error message is pretty cryptic and looks like it hit an edge-case. (The error I fed it was: java.lang.ClassCastException: Cannot cast jdk.nashorn.internal.runtime.Undefined to java.util.Collection)

Yes, this is caused by providing an object, which is not an Error object. I wanted to check, if I got an Error object or a child object of Error object, but knew this will be tricky. So I just made a "cheaper" implementation first but then forgot to revisit it.
I think it is not an even an edge-case, but something many people might do: Just temporary log out an object for debugging purposes, to figure out what is going on.
I will provide a fix.

Fixed this.
Was easy, I was just thinking too complicated at the time.

Core/automation/lib/javascript/core/log.js

jpg0 · 2019-10-22T22:05:18Z

@openhab-5iver sorry I never responded to your original questions:

@jpg0, you had a library for logging in your test commit. Does the functionality in this PR align with yours? Do you have any suggestions or improvements?

The logging library that I had was actually just some handy utilities that I was using, but wasn't really a logging library. The only function that it had over this was the ability to pass in the level in case it needed to be dynamic: log.atLevel('warn', 'message'), but I'm not concerned if this isn't present as it's a rare use-case and it's easy to wrap this library if needed.

Looking at this contribution, I think most of the things that I thought about are already being covered. The only other one is the whole notification functionality. I'm undecided about that. I do think that it's useful to be able to notify of errors, however this seems like a weird place to put it. Typically you wouldn't do it like this as it's pretty high coupling and at a high level in the stack, but would instead expect something like:

use a chain of responsibility pattern to allow adding notifications if required
use an event-based system to reduce coupling further, allowing notifications to subscribe to applicable log events to also send them as notifications (possibly async events here)
just do notifications as a log Appender and stick it into the log system so that all of OH gets this functionality
But TBH I'm not too concerned; given that this doesn't affect the API for scripts its something which can be changed or moved at a later date.

Also, as for the compatibility with a move to CommonJS, this works fine and I've already switched over to using it. I agree that we should remove all the logInfo etc, functions, and need to think about what we will do with console.log (I do like that it's defined as it will allow existing 3rd party nodeJS code to work).

jpg0 · 2019-10-22T22:13:52Z

Core/automation/lib/javascript/core/log.js

+            if (config.DEBUG.prefix === undefined) config.DEBUG.prefix = "short";       
+
+            return Object.create(Object.prototype, {
+                _notificationLevel: { value: (notificationLevel === undefined || notificationLevel === null) ? context.NOTIFY_OFF : notificationLevel },


You are using an underscore prefix for private vars. @openhab-5iver is this a coding style that we should be using in general?

Also note that these vars are still accessible; if you want to actually make them inaccessible, I'd suggest that you move the definition of these vars from the object block into the containing function body.

Yes, I am aware of that and even did it this way originally. But then I got worried if this would not cause an issue when I instantiate multiple Loggers as I got confused by all these JS context mechanism. So I just moved it into the returned object, well aware that I loose the privacy.
Maybe I should have just taken the time and test it out. I will do actually do this and move them into the function body, if it works.

@openhab-5iver is this a coding style that we should be using in general?

That would be my preference. That's how we do it in Python, so it fit in perfectly IMO!

As long as the vars are defined in the function, they should map 1:1 to the object being returned so you should be fine.

@openhab-5iver I can certainly see the bias for Python here! I agree that striving for alignment is good. I would only caution that there are likely to be areas requiring divergence if you want the JS support to be idiomatic to JS, not Python (although I don't think this is one of them!).

Truly, it is not bias. I'm attempting to keep some consistency in place to ease the future migration to a Scripting API, which will absorb pretty much all of the helper library functionality. That will take some time though, and development is continuing. I'm hopeful that, with the new development efforts on the JS libs, there will be some collaboration on how to move forward with bringing their functionality to the level of the Python libs.

I have done plenty of development, but my background is from the management side of things. I'm currently the only active maintainer in this repo, and I am very open to collaboration on its technical direction, especially with the JS libs!

5iver · 2019-10-22T22:25:47Z

The only other one is the whole notification functionality. I'm undecided about that. I do think that it's useful to be able to notify of errors, however this seems like a weird place to put it.

I agree. I put it into Jython core.log to test and almost forgot it was there, but was reminded by a random error that showed up on my phone. It's caught several that I probably would have missed. I'm sure there is a better way, but I've been glad that I put it in.

jpg0 · 2019-10-23T00:44:59Z

After using this library for quite a bit, I wanted to raise a UX issue: the prepending of location can be a positive, but can also be a big drawback when log statements start in differing columns.
The current OH logging includes namespace, but truncates (or pads) them to ensure that log messages always begin at the same column. This is not the case with this library and I'm having a really hard time reading debug logs because I cannot scan down the messages because they begin at different places on the screen. I would suggest two options:

(hard) use a fixed size string to include location information, just like OH does
(easy) provide a global toggle to override location information on and off - similar to how the npm debug package works.

Martin-Stangl · 2019-10-23T16:17:33Z

I agree to the UX issue. I myself noticed that it sometime is difficult to quickly see the actual message as there so much stuff is going on before it.
I will try something else first: I will will shorten the filename and put the location details at the end instead.

Log message format changed for better readability. Private functions actually made private. Blank line added after end of code. Logger name prefix aligned with LogAction. Signed-off-by: Martin Stangl <[email protected]> Signed-off-by: Martin Stangl <[email protected]>

Martin-Stangl · 2019-10-23T20:50:39Z

The only other one is the whole notification functionality. I'm undecided about that. I do think that it's useful to be able to notify of errors, however this seems like a weird place to put it. Typically you wouldn't do it like this as it's pretty high coupling and at a high level in the stack, but would instead expect something like:

use a chain of responsibility pattern to allow adding notifications if required

use an event-based system to reduce coupling further, allowing notifications to subscribe to applicable log events to also send them as notifications (possibly async events here)

just do notifications as a log Appender and stick it into the log system so that all of OH gets this functionality

I agree, this should be decoupled.
This tight coupling resulted from this initially being just a helper function for me. Simple and easy to use. It got already a bit more sophisticated, when I decided to make it public and meanwhile it is reaching a point where having logging and notification combined is making things a little messy.

Right now I am thinking in the following direction:
Create a console.js, (similar to what node.js has), but instead of it logging directly, log.js and notification.js can subscribe to console.js.
The two things I have still to figure out:

One instance of Console should be initialized by default, ideally with no load/require needed at all (as node.js does), so console.log() just works out of the box. A Logger with default config will be initialized with this Console instance and subscribe to all Console log events of Console to get the messages into the log by default.
Some clever subscription management: I personally would use my own instances of Console instead of the default one and suggest this for each core library as well. Simply to have different logger names, so log levels can be controlled per library. But I guess this would require features like "subscribe to error messages of any Console instance" and "have the default Logger automatically subscribe to my new Console instance.

5iver · 2019-10-24T17:01:14Z

After using this library for quite a bit, I wanted to raise a UX issue: the prepending of location can be a positive, but can also be a big drawback when log statements start in differing columns.

@jpg0, could you please include an image of what you mean? I have commented out the "Common pattern layout for appenders" section at the top of the org.ops4j.pax.logging.cfg file, so my logging looks different than stock...

jpg0 · 2019-10-25T00:43:35Z

Sure; here's a sample:

Often I'm looking for a specific log statement about what I'm working on. When the statements don't start at the same horizontal location it's very hard to scan down to find the statement that you're after.

Martin-Stangl · 2019-10-25T15:47:18Z

@jpg0 please let me know how you like the new format, once you had a chance to try it out. Thanks.

jpg0 · 2019-11-01T00:15:41Z

Core/automation/lib/javascript/core/log.js

+                        case "info":
+                        case "debug":
+                        case "log":
+                            var stackArray = msg.stack.split('\n\tat ');


Can we use /\s+at\s+/ rather than '\n\tat '?
The current version is very specific, and (for example) fails on GraalJS which uses spaces rather than tabs.

I think we should be able to adjust it to also work with GraalJS.
But we have to be very specific, as the stack also contains the message text, which can be whatever the user wants it to be, including multi-line.
Therefore I think your suggestions is way too unspecific, as it would grab occurrence of the word "at".
Worst case, I might need to identify the engine we are running on and define a regex for each specifically.

Oh, good point. I just dropped that in to keep it working in my env. I would expect that ensuring it's the start of the line /^\s+at\s+ would be better, or even just fixing it to tab OR 4x space /^(\t| )at\s+ (<- github butchers the 4 spaces in this, but you know what I mean). Personally I wouldn't try to detect the engine.

jpg0 · 2019-11-01T00:16:50Z

Core/automation/lib/javascript/core/log.js

+            }
+
+            var _getCallerDetails = function _getCallerDetails (msg) {
+                var matches = msg.stack.split('\n\tat ')[3].match(/(.+?) \((.+):(\d+?)\)/);


Same comment as below - this stack parsing is specific to Nashorn; /\s+at\s+/ should work for Nashorn + others.

Also, same comment as below.

jpg0 · 2019-11-01T00:25:09Z

@Martin-Stangl aplologies for the delay; been busy this week. Anyway, yes I much prefer this.

I did add a couple of comments regarding stack parsing, which make this code fail on Graal (I've managed to get this working; I'll submit a PR once I'm happy with it all). Additionally the fileName property on an error is not present in Graal. Up to you if you want to attempt to handle these at this point.

Also, one other question: why did you do this as an object with Object.create, rather than just a closure with error/warn/etc as methods in the returned object? Was it to try to allow polymorphism in the future? I ask because this style I find much more complex (and that's even with me coming from Java and initially attempting to OO all my JS).

Martin-Stangl · 2019-11-01T19:51:44Z

Additionally the fileName property on an error is not present in Graal. Up to you if you want to attempt to handle these at this point.
fileName, lineNumber and stack are all not part of the ECMAScript standard. So ideally I should not use any of it, but that leaves me with nothing. I guess I will integrate a fallback to pull it out of stack and maybe another fallback if even stack is not available?

Also, one other question: why did you do this as an object with Object.create, rather than just a closure with error/warn/etc as methods in the returned object?
For once, because we only have ES5 :-P
I wanted to have private methods and variables and initially could not find another way to do it or at least I also understood. So meanwhile, all my stuff is built like that. I did not went so far to need polymorphism (although I think I have a first proper use case at hand: lights - some are HSBtypes, some On/Off), but I make use of the possibility to define which variables are writable and which not.

jpg0 · 2019-11-01T21:50:05Z

I noticed that while Graal doesn't have the fileName property (although it does have stack), you can get the filename from the stack. For example, here is a stack trace:

org.graalvm.polyglot.PolyglotException: TypeError: Cannot read property 'split' of undefined
        at <js>._getLogMessage(log.js:55) ~[?:?]
        at <js>.info(log.js:178) ~[?:?]
        at <js>.context.JSRule(rules.js:110) ~[?:?]
        at <js>.:anonymous(<eval>:35) ~[?:?]
        at <js>.:program(<eval>:4) ~[?:?]
        at org.graalvm.polyglot.Context.eval(Context.java:344) ~[?:?]
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:346) ~[?:?]

Not sure if that is useful! I do think at a minimum being defensive over the property not being present would be worthwhile (at least for me right now!).

jpg0 · 2019-11-01T21:58:12Z

Regarding the class construction stuff, what I meant was instead of:

<in Logger function>
var _privateFunction = ...
...
return Object.create(Object.prototype, {
                publicProperty: {value: "foo"},
                ...
                publicFunction: { value: function publicFunction (param) {
                   //impl
                }},
                ...
}

to use a closure, like the other existing files (by lewie I gather) use:

<in Logger function>
var _privateFunction = ...
...
return {
                publicProperty: "foo",
                ...
                publicFunction: function (param) {
                   //impl
                },
                ...
}

(Not sure what your dev background is, but I had primarily Java experience before I started with JS, but was quickly shown to stop trying to make everything OO and embrace closures and functional style, and only use classes for specific cases.)

Martin-Stangl · 2019-11-02T21:04:30Z

Thanks, I must have totally failed to see this approach in lewie's code. I actually tried to copy lewie's style before doing something else, but back then my JS knowledge did not allow me to understand it all too well. (Not that I became an expert in the past 2 weeks...)

Yes, this would get rid of Object.create(Object.prototype.

I think the other stuff is the exact same thing: It should be possible to use publicProperty: "foo" instead of publicProperty: {value: "foo"}, also in Object.create. I actually considered to change this, once I figured out that all my functions where called value in the stack trace due to that notation if I do not name them, but have the proper name when using your shorter notation.
I wonder if there is also a different notation, if want to make a variable writable, which is easier to read. I am sure there is a way to do it differently, but have to see, if it has better readability.

So I am wondering, if it is an advantage to keep Object.create around as the rest should be adjustable to your suggested style. At the moment I have no arguments for or against it.

(I guess my dev background has not much to do with the choice. I already coded in 25+ languages but programming stopped being my main business over a decade ago. And back than I mostly worked in SAP ABAP, before it knew about any OO concepts - and old ABAP does not translate to JS very well.
So in the rare occasions when it happens that I find time to do some programming, I basically have to start from scratch and google for every command how it is named an how it is used. And in this case I only knew I wanted something that I could best describe as an object and ended up with Object.create after some googling. Mostly because the blog explaining it was probably the best to explain it to me.)

jpg0 · 2019-11-02T22:19:58Z

The only benefit I know of explicitly using Object.create is that it allows you to develop an object hierarchy if you want polymorphism. I don't think it's necessary here though so I'd drop it.

I wonder if there is also a different notation, if want to make a variable writable, which is easier to read.

I don't understand this? All the variables here are writable AFAIK? var will mean that it's writable, you need to use const (which won't work with Nashorn, but will with Graal) to make it readonly. Object properties are typically always writable.

Martin-Stangl · 2019-11-04T15:45:13Z

Sorry, my wording was not precise: I am referring to the object properties.
For e.g. if I do this:

var log = Logger("MyLoggerName");
log.name = "MyNewLoggerName";

It will throw the following error:
16:33:35.231 [ERROR] [ript.internal.ScriptEngineManagerImpl] - Error during evaluation of script 'file:/C:/WORKSP~1/Projects/openHAB/openhab/conf/automation/jsr223/javascript/personal/test.js': TypeError: "name" is not a writable property of [object Object] in <eval> at line number 17

The reason is that all object properties are write protected by default and I did not specify anything changing this:

            return Object.create(Object.prototype, {
                name: { value: _name },

If I would want name to be writable, I would need to change it to:

            return Object.create(Object.prototype, {
                name: { value: _name, writable: true },

I really like this, as I can make some information available without additional effort, but still be ensured that nobody changes my variables unexpectedly. Otherwise I would need to make these properties private and write getters.
There are also a few other parameters, but I did not make use of them yet. See Object.defineProperty()

jpg0 · 2019-11-06T01:21:11Z

Ah, I wasn't aware that you were trying to expose readonly properties. TBH I have never really used these as I've not really seen them used much in APIs so haven't used them myself. If you want to retain them, then yes you'd need to keep this style (or alternatively Object.freeze the object before you return it).

Also: another question as I'm regularly butchering the code to ensure it works as a module:
Do you anticipate that Logger is the only top-level function that you will expose? Because if so I'll make it the default export so that you can create loggers like this:

const log = require('log')('myscript');
log.info(...);

Much nicer than require('log').Logger('myscript'); IMO.

Martin-Stangl · 2019-11-06T07:35:48Z

I tried to simplify the style based on your example but with Object.create, but it did not work. So my assumption above was wrong and I guess it has to stay as it is.
Object.freeze or Object.defineProperty would be an option, but it means freezing every object in my returned object separately. Although, if I do not care about the functions, it would not come up that often.

Yes, I intend to keep it the only one exposed function.

Martin-Stangl · 2019-11-06T07:52:35Z

@openhab-5iver: I really like your idea of creating a console log functionality. But right now I have to focus on actually doing my rule implementations and then a different project. So I will not come around to it soon.

But I could wrap up the current state by replacing the core library logging with this logger in a way that it would be compatible to a future actual console implementation.
This means

call the log object console
only use console.error, console.warn, console.info and console.debug
no use of {} replacement
no use of the notification functionality

For documentation I could document the full feature set or only the console compatible usage for now.
It would even be an option to rip out any functionality, which does not conform with console and provide it as console.js, or at least wrap the logger in a core console object. I like the idea more and more as I type this out.

The other approach would be to be logAction compatible, what we basically are, if we ignore the log.trace and notification functionality.

What do you think?

Question is also on what to do with the community stuff. This all would probably break. Should we update this too or leave it to the creators?

jpg0 · 2019-11-07T01:01:17Z

@Martin-Stangl re: Object.freeze, I think you only need to change your code to:

return Object.freeze(Object.create(Object.prototype(...

ps. now I'm using ES6, all the class-based stuff is far nicer!

jpg0 · 2019-11-07T09:41:24Z

@Martin-Stangl I've just ported this to ES6 as it kept failing on Graal stacktraces; in case you're interested it's here: https://gist.github.com/jpg0/5c991b486255b015221a04056548a74b (I removed notifications in favour of providing a callback which could process them).

Martin-Stangl · 2019-11-07T22:44:16Z

@Martin-Stangl re: Object.freeze, I think you only need to change your code to:
return Object.freeze(Object.create(Object.prototype(...
ps. now I'm using ES6, all the class-based stuff is far nicer!

It would almost work. Object.freeze does not freeze objects in the object, so config would remain writable.
The problem is described here and they also show a simple deepFreeze() function to handle this.
And not relevant for log.js, but I use the same pattern in all my "classes" in my rules, so I have cases, where I keep some properties writable. So I would also need to consider a feature to specify which properties to freeze and which not.
This could definitely be put into a freezer function. I would not be surprised, if someone out there already made it.

jpg0 · 2019-11-07T22:55:19Z

That's true, but I was just trying to emulate what you've already done - right now making the config a readonly property still allows consumers to mess with what's inside it. My personal view on this is that it's not worth exposing at this point, as the use-cases for getting something that you've just set are pretty slim for a logger. For simplicity, I wouldn't expose anything that is passed in until (a) it's really needed or (b) it becomes much easier with ES6.

jpg0 · 2020-04-01T09:12:15Z

Thought that I'd leave a comment here as I finally got around to adding custom appender support to my logging module, for another approach on how it could be done.
I decided that the simplest way to do it was to have the log library search for a predefined module of appenders when it loads. The code is here and can be used by dropping in a 'log_appenders.js' file with content such as:

module.exports = {
    forLevel: level => {
        switch(level){
            case "error":
                return {
                    logAt: (level, msg) => sendLogErrorLineSomewhere(msg)
                }
            default:
                return null;
        }
    }
};

I think it strikes a good balance between not hardcoding any specific appenders (for mine I have error messages going to a telegram group for example), performance (very little code in the logging hot path) and simplicity.

initial contribution core/log.js

995c762

Martin-Stangl mentioned this pull request Oct 18, 2019

Use a JS module system for JS rules #267

Closed

log string added, fixed parameterization for notifications, legacy co…

f25bf61

…nsole log functions improved/fixed

Revert master back to upstream state.

91a89a0

5iver requested review from lewie and 5iver October 19, 2019 17:14

This was referenced Oct 21, 2019

Added initial CommonJS support #270

Closed

Added initial (unisolated) CommonJS support #271

Closed

5iver requested changes Oct 22, 2019

View reviewed changes

Martin-Stangl added 3 commits October 22, 2019 21:21

Fix: Wrong check for Error object

067dac6

Signed-off-by: Martin Stangl <[email protected]>

Merge branch 'master' of https://github.com/openhab-scripters/openhab…

45ea7e4

…-helper-libraries

Merge branch 'master' into feature-logger

7d5ea41

jpg0 reviewed Oct 22, 2019

View reviewed changes

Core/automation/lib/javascript/core/log.js Outdated Show resolved Hide resolved

jpg0 reviewed Oct 22, 2019

View reviewed changes

jpg0 reviewed Nov 1, 2019

View reviewed changes

Initial contribution core/log.js #268

Are you sure you want to change the base?

Initial contribution core/log.js #268

Conversation

Martin-Stangl commented Oct 18, 2019

Martin-Stangl commented Oct 18, 2019

Martin-Stangl commented Oct 19, 2019

5iver commented Oct 19, 2019

Martin-Stangl commented Oct 22, 2019

jpg0 commented Oct 22, 2019 • edited Loading

Martin-Stangl commented Oct 22, 2019

5iver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5iver Oct 22, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Stangl commented Oct 22, 2019

jpg0 commented Oct 22, 2019

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

5iver commented Oct 22, 2019

jpg0 commented Oct 23, 2019

Martin-Stangl commented Oct 23, 2019

Martin-Stangl commented Oct 23, 2019 • edited Loading

5iver commented Oct 24, 2019 • edited Loading

jpg0 commented Oct 25, 2019

Martin-Stangl commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpg0 Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpg0 commented Nov 1, 2019

Martin-Stangl commented Nov 1, 2019

jpg0 commented Nov 1, 2019

jpg0 commented Nov 1, 2019

Martin-Stangl commented Nov 2, 2019

jpg0 commented Nov 2, 2019

Martin-Stangl commented Nov 4, 2019 • edited Loading

jpg0 commented Nov 6, 2019

Martin-Stangl commented Nov 6, 2019

Martin-Stangl commented Nov 6, 2019 • edited Loading

jpg0 commented Nov 7, 2019

jpg0 commented Nov 7, 2019

Martin-Stangl commented Nov 7, 2019

jpg0 commented Nov 7, 2019

jpg0 commented Apr 1, 2020

jpg0 commented Oct 22, 2019 •

edited

Loading

5iver Oct 22, 2019 •

edited

Loading

Martin-Stangl commented Oct 23, 2019 •

edited

Loading

5iver commented Oct 24, 2019 •

edited

Loading

jpg0 Nov 1, 2019 •

edited

Loading

Martin-Stangl commented Nov 4, 2019 •

edited

Loading

Martin-Stangl commented Nov 6, 2019 •

edited

Loading