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

Allowing use of both meta and splat in same call to log() #1485

Merged
merged 1 commit into from
Sep 29, 2018

Conversation

mpabst
Copy link
Contributor

@mpabst mpabst commented Sep 29, 2018

The arguments.length > 2 section in log() examines the first item in the splat param to see if it's a meta object. Problem is, a subsequent call to splat.slice() still includes that meta object in the splat, accidentally calling slice(0) instead of slice(1).

Here's an example that breaks under the merge target but works when using the merge source:

const winston = require('winston');

let transports = {
  console: new winston.transports.Console({level: 'info'})
};

let {format} = winston;
let logger = winston.createLogger({
  format: format.combine(
    format.splat(),
    format.printf(info => `[${info.label}]: ${info.message}`)
  ),
  transports: [transports.console]
});

logger.log(
  'info',
  'let\'s %s some %s',
  {label: 'label!'},
  'interpolate',
  'splat parameters'
);

The arguments.length > 2 section in log() examines the first item in the splat param to see if it's a meta object. Problem was, a subsequent call to splat.slice() still included that meta object in the splat.
@mpabst
Copy link
Contributor Author

mpabst commented Sep 29, 2018

Here you go @DABH – I just went and created a new PR, as you can see. Thanks for checking this out!

@DABH
Copy link
Contributor

DABH commented Sep 29, 2018

Thanks for the example, that helps clarify things! :) I took a look and this really seems like a silly bug (why would you call splat.slice(0) and not just splat? clearly the intent was to remove meta from splat, hence should be splat.slice(1)). Going to merge this, and it should land in the next release in about a week! Thanks again for your patience and contribution!

@DABH DABH merged commit af941f4 into winstonjs:master Sep 29, 2018
@mpabst
Copy link
Contributor Author

mpabst commented Sep 30, 2018

Great, thanks again for doing so much for this project! 😊🙌

DABH added a commit that referenced this pull request Oct 9, 2018
@DABH
Copy link
Contributor

DABH commented Oct 9, 2018

Actually this seems problematic -- I can no longer log a single object as a splat:
logger.info('hey %O', obj);
doesn't format the splat right anymore...

@DABH
Copy link
Contributor

DABH commented Oct 9, 2018

@mpabst how would you feel about putting your extra meta objects after your other splats? I feel like that is more logical and the way it may have been intended to work... would love your feedback on
#1499
winstonjs/logform#57
! Thanks!

@mpabst
Copy link
Contributor Author

mpabst commented Oct 16, 2018 via email

indexzero pushed a commit that referenced this pull request Dec 7, 2018
indexzero added a commit that referenced this pull request Dec 23, 2018
indexzero added a commit that referenced this pull request Dec 23, 2018
indexzero added a commit that referenced this pull request Dec 26, 2018
…on & logform (#1552)

* [doc fix] Build on the work from #1485 fixes.

* [tiny] Formatting changes.

* [fix test] Updated tests to latest (consistent) semantics.

* [dist] Update to `logform@2.0.0` and `winston-transport@4.3.0`.

* [doc] Document all possible permuations for splat.
Mizumaki pushed a commit to Mizumaki/winston that referenced this pull request Jun 11, 2020
The arguments.length > 2 section in log() examines the first item in the splat param to see if it's a meta object. Problem was, a subsequent call to splat.slice() still included that meta object in the splat.
Mizumaki pushed a commit to Mizumaki/winston that referenced this pull request Jun 11, 2020
Mizumaki pushed a commit to Mizumaki/winston that referenced this pull request Jun 11, 2020
…on & logform (winstonjs#1552)

* [doc fix] Build on the work from winstonjs#1485 fixes.

* [tiny] Formatting changes.

* [fix test] Updated tests to latest (consistent) semantics.

* [dist] Update to `logform@2.0.0` and `winston-transport@4.3.0`.

* [doc] Document all possible permuations for splat.
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.

None yet

2 participants