Skip to content
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

Redo log #1404

Closed
wants to merge 54 commits into from
Closed

Redo log #1404

wants to merge 54 commits into from

Conversation

LionC
Copy link
Contributor

@LionC LionC commented Oct 15, 2021

This completely replaces the log module with a module trying to achieve the following goals:

  • Have a very easy to set up global default logger
  • Allow custom log levels with a minimal, clean API
  • Adhere to the console-ish standard of <logger>.<level>(message) also for custom log levels
  • Have a standard way for frameworks to log
  • Allow complete customization but have sane preconfigured options out of the box

The README and doc.deno.land on the module should give an overview.

The design of this module is heavily influenced by my discussions with @timreichen and input by @caspervonb .

This is marked as draft because it is still missing tests - I prioritized finishing docs so the picture is complete, as I promised @kt3k to post this tonight ;-)

@LionC
Copy link
Contributor Author

LionC commented Oct 15, 2021

Some notes: I typed most of the docs on the train and will not be able to fix the resulting typos (one of the broken Macbook keyboards -.-) or words I have missed while revising until my return on Monday.

@LionC
Copy link
Contributor Author

LionC commented Nov 15, 2021

From the talks we had about this, I think Tim and I agree on the overall composition concepts (log = dispatch -> handle, if you want multiple independent handlers there is some form of multi-logger that is arbitrarily nestable). Also as we have read each others proposals back and forth now and incorporated each other ideas into the different versions, this will be a collaborative result and a definitive improvement over the current module no matter what - we just have the luxury to decide between two versions :-)

I added my (unnecessarily long, bikeshedding yay^^) perspective on the actual differences between the two proposals below - ultimately, the first point is the most important for me, because that one potentially affects all users who will ever log, not just logger authors.

The API the loggers (not the module) expose

This proposal sticks to the console-ish standard no matter the log levels, so if you use custom log levels, say audit, log, critical, the API to the users of your logger is

logger.audit("Things happened")
logger.critical("Something went deeply wrong")
logger.log("Someone did a thing that you should know about")

The goal here was to make the actual logging part as concise as possible, because loggers get called a lot and those calls are all over the place in a codebase, while also sticking to a web standard(-ish) that JS devs are very familiar to.

Tims proposal (at least when we discussed this and when I played around with his branch) did not do this, because, as far as I have explored, there is just no way to do that in a type safe manner with classes without having some wrapped API (which defeats the whole purpose of not having those).

To class or not to class

For me, passing around callbacks is not a weird thing in JS - from .setInterval(), .map() etc. to event or request handlers, passing a function to something to give it behaviour has never been something that I saw JS devs struggle with. Classes on the other hand, especially if we talk about abstract classes and inheritance, have pretty complex semantics, verbose code and several pitfalls - which does not mean that one should never use them, but with JS being a style agnostic multi paradigm language, we have the luxury to choose the "right" tool for each job.

In my mind, a logger basically boils down to a callback. Someone logs a message, the callback handles it. All of a loggers behaviour is that callback. The only thing that differentiates a logger from any function, is that it has the concept of levels with a threshold filter (in this proposal, the dispatcher), which in almost all use cases will never be touched by consumers of the module (they will often use the default levels and almost always just set some static threshold and probably not be aware that a dispatcher exists).

Yes, that can be modelled as a class and in Java it would definitely be. But Classes expose a very large API surface - that of writing a class with all options and syntax and pitfalls and implications - to someone who wants to write a logger, while we know that they very very likely will just want to set one callback to handle messages.

I would personally also not call these functions factory functions - I do not think the term makes a lot of sense in the context of JS, as anonymous objects are first class citizens in JS (and have been around longer than "real" classes). Factory functions (as a JVM term) are a workaround for complex cases around classes, while functions that return new objects are a very common idiom in JS and not a workaround at all.

So in the end, for me both approaches are valid at their core, but the functions expose a more streamlined, smaller API and generate less code in the module and for consumers writing loggers.

The rest

There are also some features that this proposal implements that are missing in the class based one - most importantly the concept of framework logging, which is a huge and important step to set a starting point that could benefit the ecosystem significantly down the line - but those are not too relevant for the decision, as they can be implemented in both approaches - that would have to be done though.

@LionC LionC marked this pull request as ready for review December 5, 2021 20:47
@LionC
Copy link
Contributor Author

LionC commented Dec 5, 2021

I think this is ready now. I gathered some more feedback and applied several changes:

  • As suggested by @bartlomieju , the default logger now logs to the console, together with some simple functions to configure that behaviour
  • Third party (library / framework) logging is now an actual feature, including configuration functions
  • Finished tests, including subprocesses for console logging and more stability by excluding timestamps in them

I think the pipeline is failing because the current import mechanism in code snippets does not work if the token does not exist in the current release - is that intended or am I missing something?

@iuioiua
Copy link
Collaborator

iuioiua commented Nov 21, 2022

Gentle ping - what's the latest update on this PR?

@ry
Copy link
Member

ry commented Nov 22, 2022

closing because stale

@ry ry closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants