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

Added support for passing a function/closure to the logger #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cadement
Copy link

@cadement cadement commented Dec 6, 2016

Added support for passing a function/closure to the logger as a message instead of a raw string. This allows log messages to be lazily evaluated only when actually emitted (which can yield substantial performance benefits). This was already possible using logging level-based conditional logging, but the syntax was much messier:

if (logger.level.level < 5000) logger.trace(JSON.stringify(myBigObject));

vs
logger.trace(() => JSON.stringify(myBigObject));

…ge instead of a raw string. This allows log messages to be lazily evaluated only when actually emitted (which can yield substantial performance benefits). This was already possible using logging level-based conditional logging, but the syntax was much messier:

    if (logger.level.level < 5000) logger.trace(JSON.stringify(myBigObject));
  vs
    logger.trace(() => JSON.stringify(myBigObject));
@nomiddlename
Copy link
Collaborator

I can see this being useful, so thanks for the PR. My only concern is that there could be someone out there that's logging a function somehow, and this would now execute that function. Would you be able to add a config parameter to turn this feature on and off - with "off" being the default. A boolean at the top level of config would be fine.

@nomiddlename nomiddlename added this to the 2.x milestone Dec 13, 2016
@nomiddlename nomiddlename removed this from the 2.x milestone Jul 11, 2017
@nomiddlename
Copy link
Collaborator

Leaving this open because it's still a good idea, but it'll need to some work to port to tap tests and allow for turning it off and on.

@nomiddlename nomiddlename removed this from TODO in Version 2.x Nov 8, 2017
@ericnewton76
Copy link

ericnewton76 commented Feb 24, 2018

IMO this is a great feature and its more natural.

I would recommend a major version bump upon inclusion of this, and allow the config default to be true, noting that in a v2->v3 upgrade guide

Or include this in current version with default off, but make sure to make default on in eventual log4js@3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants