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

Split code for main/renderer, refactor and new features #19

Closed
wants to merge 5 commits into from

Conversation

cgalvarez
Copy link
Contributor

I've tried my best to keep the code as short and concise as possible, and not adding new external deps (the only exception is the package callsites, maintained by @sindresorhus), but this PR has become something really big 😅

Any insights/help will be greatly appreciated. Still missing some features that I would like to include.

New options

  • collector (false, "main", "renderer" or id; defaults to "main"): the logger that will collect all the logger messages. All messages are logged on their original logger apart from the collector. This implements Add option to show main process output in the renderer #15 :
    • false: removes any collector and each side (main/renderer) collects its own messages.
    • "main": all logged messages are collected on timber [main] (default behavior until now).
    • "renderer": all logged messages are collected on the first created browserWindow.
    • id: all logged messages are collected on the browserWindow with ID id after it is created (all messages logged prior to its creation are only logged on their respective loggers).
  • devToolsDarkTheme (false or true; defaults to false): true if electron app is using the devTools dark theme, false otherwise. When prettifying the logged message, all colors are check at renderer side when internally cached to improve the contrast, so the messages are legible.
  • muteElectronInspector (false, true or a level priority (0=error, 1=warn, 2=everything else); defaults to false): allows muting the messages printed by Electron. Combining this option with ignore, user can mute messages with a very fine-grained control. This closes Filter out Electron junk logging #13 .
  • prettify ("none", "context" or "all"; defaults to "context"): the logged text can be colorized on both sides (main/renderer). If a transport does not support this feature, defaults to none. Possible values:
    • none: strips all colors.
    • context: colorizes only the context part (logger [side] >).
    • all: colorizes the context and all the arguments passed to the logging method.
  • separator (any non-empty string; defaults to "›"): the separator used to separate the context and the message.
  • timestamp (false, "iso" or "time"; defaults to false): messages can be prepended with a (ISO formatted or only time) timestamp on demand, which defaults to false, and may have as possible values false, 'iso' or 'time'. This feature is only available for main logger because user can manually activate this on renderer devTools under Console settings > Show timestamps.
  • transports ((string|Object)[]; defaults to ["console"]): the transports to set up for all loggers.

New features

  • User can now setDefaults() from renderer process(es) too.
  • Loggers with the same name (no matter on which side, main/renderer) share the same options (which may be overriden by instance-specific options at creation time), inheriting from existent instances with the same logger name.
  • New mechanism to load transports on demand, which helps isolating stuff and prepare for Add ability to also log to disk #7 (add ability to also log to disk). Right now, if no transport is provided, the ConsoleTransport is always used as default (which is the unique currently available).
  • The default log levels are now error (0), warn (1), info (2) (with log as alias), verbose (3), debug (4), silly (5). So it's backwards compatible and provides new methods for more fine-grained control. User can set its own log levels through the new public method setLevels().
  • Default logger is named timber, and if no name provided when creating a custom logger, then electron-timber tries to find out the caller package and extract its name from its package.json file. If not found, it will throw an error.
  • Now all args are prettified on both side consoles (main/renderer). If any of the args is not a string, then it's printed as object on renderer console(s) to allow inspection or prettified on main (terminal) console to ease inspection.
  • Every logged message is now prepended with its context, wich is the logger name plus its side (and optionally the browserWindow ID on renderer side). Examples: timber [main], myCustomLogger [renderer 2]. This eases a lot debugging multi-window apps and distinguishing one logger from another after fixing Add option to show main process output in the renderer #15 .
  • TODO Handle (on both sides), prettify (on main process like pretty-exceptions) and collect (through collector option) uncatched exceptions.
  • TODO Allow overriding defaults through command line parameters (like TIMBER_LOGGERS: TIMBER_HOOK_CONSOLE,...).

Internals/refactor

  • console stuff moved to its own AbstractConsoleTransport class, extended by MainConsoleTransport and RendererConsoleTransport.
  • All constants and literals moved into constants.js and imported where/when necessary.
  • The log level helper methods (error, warn, info, log and so on) are created, optimized and binded dynamically now (these are inferred from the log levels, global defaults and other options).
  • TODO Update README.md.
  • TODO Adapt (old) and write (new) tests.

Fixes

  • Measures taken with time/timeEnd were always being logged previously. Now they are logged based on the logger logLevel (they default to be printed with info/log level).
  • Chromium devTools/extensions are loading the preload script, so they usually cause Filter out Electron junk logging #13.
  • Relying on electron-utils.appReady.then() was missing some browserWindows because of its asynchronous behaviour.
  • The logger context was not being added to messages from streams.
  • The messages captured from browser (renderer) native console calls were not being stripped custom CSS (whenever present).
  • The context (and thus the logged messages) is correctly padded on renderer sides now.

Closes

Some screenshots of this PR in action

Collector as "main" with multiple browserWindows, hooking native console and prettifying "all":

timber - main console

Collector as "renderer" with multiple browserWindows, native console hooked, "all" prettified and devTools with dark theme:

timber - renderer console

Copy link

@xxczaki xxczaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably fix linter errors ;)

@cgalvarez
Copy link
Contributor Author

cgalvarez commented Aug 28, 2018

@xxczaki I completely agree with you 😄 I will try to fix them ASAP.

@sindresorhus Maybe it would be better to create a dev branch and make this PR against it, so more people could help with the tests suite before merging into the master branch?

@cgalvarez
Copy link
Contributor Author

All linter errors fixed. Only remain 4 warnings due to TODO/FIXME comments which I want to address before merging (the catch/prettify unhandled exceptions feature could be done in a separate PR).

@sindresorhus
Copy link
Owner

I will try to find some time tomorrow to look through the code. It's a lot of changes.

Looking at the output, it's not clear to me what the [0] prefix means? It's also not clear to me what the croupier vs timber indicators mean.

Right now it shows a unique name for each renderer, like renderer 1, which is a good default, but in most of mine Electron projects, I only have one window, so I would like the ability to set that all renderers should have the same name, renderer.

@cgalvarez
Copy link
Contributor Author

cgalvarez commented Sep 10, 2018

Looking at the output, it's not clear to me what the [0] prefix means?

That part of my output has nothing to do with electron-timber. I use nodemon in my dev gulp pipeline to watch some config files. It only means that electron is running on the first spawned process. As you can see, it does not appears on the renderer console.

It's also not clear to me what the croupier vs timber indicators mean.

I create two different loggers in my project. croupier is a separate NPM package that uses electron-timber, and that is its own logger. There is another logger that doesn't appear in the screenshots with a different name for the app itself. timber is the default logger created by electron-timber. When you see any timber-prepended message it can mean three things:

  1. The developer is using the default electron-timber logger directly.
  2. A native console call that is hooked and captured. Those are always printed as timber logger because I have no idea of what custom logger to assign (in my case, I use multiple).
  3. A message captured from a devTools inspector that it's not binded to any browserWindow (those targeted with the option muteElectronInspector).

I would like the ability to set that all renderers should have the same name, renderer.

I think the best decision would be adding a new option multiwindow, which defaults to false, so it always shows as logger [renderer] unless explicitly setting multiwindow: true.

@sindresorhus Let me know if I've clarified your doubts and what do you think about the multiwindow approach 😉

@sindresorhus
Copy link
Owner

I create two different loggers in my project. croupier is a separate NPM package that uses electron-timber, and that is its own logger. There is another logger that doesn't appear in the screenshots with a different name for the app itself. timber is the default logger created by electron-timber. When you see any timber-prepended message it can mean three things:

Since all loggers are registered upfront, can you make it not show the column if there is only the timber logger? Showing the logger names when there is only one logger would be wasteful.

I think the best decision would be adding a new option multiwindow, which defaults to false, so it always shows as logger [renderer] unless explicitly setting multiwindow: true.

👍 But the option name is not clear enough. It needs to be something more explicit.

@sindresorhus
Copy link
Owner

I'm really sorry. I completely forgot about this PR. I looked at it many times, but it's a very big PR and it pretty much changes everything, so it's both time-consuming and daunting to review. Before I spend time on this, @cgalvarez are you still interested in finishing this and getting it merged?

@sindresorhus
Copy link
Owner

Closing as this PR has not seen any activity for a long time. I'm more than happy to reopen if you ever get a chance to finish this though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants