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

Alternative fix for #1501 #1539

Merged
merged 2 commits into from
Jan 29, 2019
Merged

Alternative fix for #1501 #1539

merged 2 commits into from
Jan 29, 2019

Conversation

indexzero
Copy link
Member

@pahan35 interested in your thoughts. Your fix could also work, but this one is more performant.
Assuming implicit argument values is also quite dangerous, but since the level helper methods (e.g. logger.silly('wat ok')) were created to be a little magical it seems reasonable to assume that these two statements are equivalent:

logger.silly();
logger.silly('');

Where our fixes differ is that this behavior will still crash someone's program:

logger.log('silly');

It would crash because the log method is not being invoked with the correct number of arguments – which avoids having to add additional variable-arity or type checking into the core log method hot path.

If there are no objections will add some tests for this and push it out in the next release.

@pahan35
Copy link

pahan35 commented Nov 29, 2018

I guess we can prevent multiple return by

if (args.length === 0) {
  args.push('')
}

IMHO problem with direct log without a msg

logger.log('silly');

can be omitted, cause we have msg as a required parameter in the method declaration

@indexzero
Copy link
Member Author

Glad you agree with me about logger.log('silly').

W.r.t. your comment regarding multiple returns – returning early is faster than appending to an array. Performance is the most important aspect of these helper methods. If the code is slightly more verbose that's not as much of an issue. These methods will get invoked dozens or hundreds of times per second in a typical web server so every nanosecond counts.

@pahan35
Copy link

pahan35 commented Nov 29, 2018

@indexzero thanks for your explaining about performance.

Waiting for your fix in the published to npm version.

@indexzero
Copy link
Member Author

Once we get a test for this we can land it and publish.

@pahan35
Copy link

pahan35 commented Jan 3, 2019

@indexzero any updates?

@indexzero
Copy link
Member Author

Sorry this got overlooked in winston@3.2.0. Will make sure it gets into a patch release soon.

@indexzero indexzero merged commit b1738ad into master Jan 29, 2019
@indexzero indexzero deleted the gh-1502 branch January 29, 2019 04:39
Mizumaki pushed a commit to Mizumaki/winston that referenced this pull request Jun 11, 2020
* [wip fix] Assume message is the empty string when level-helper methods are invoked with no arguments.

* [fix] Add test for winstonjs#1501.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants