Skip to content

Better handling of new Error(string) throughout the pipeline(s). Fixes #1338, #1486#1562

Merged
indexzero merged 13 commits into
masterfrom
gh-1338
Jan 26, 2019
Merged

Better handling of new Error(string) throughout the pipeline(s). Fixes #1338, #1486#1562
indexzero merged 13 commits into
masterfrom
gh-1338

Conversation

@indexzero
Copy link
Copy Markdown
Member

There are 14 distinct entry-points for this. They are now tracked by test/formats/errors.test.js:

  format.errors (integration)
    ✓ logger.log(level, error)
    ✓ logger.log(level, error) [custom error properties]
    ✓ logger.log(level, error, meta)
    ✓ logger.log(level, error, meta) [custom error properties]
    ✓ logger.log(level, msg, meta<error>)
    ✓ logger.log(level, msg, meta<error>) [custom error properties]
    ✓ logger.<level>(error)
    ✓ logger.<level>(error) [custom error properties]
    ✓ logger.<level>(error, meta)
    ✓ logger.<level>(error, meta) [custom error properties]
    ✓ logger.<level>(msg, meta<error>)
    ✓ logger.<level>(msg, meta<error>) [custom error properties]
    ✓ Promise.reject().catch(logger.<level>)
    ✓ Promise.reject().catch(logger.<level>) [custom error properties]

This also adds new semantics to winston related to (and also fixing) #1486. From the updated README.md:

NOTE: any { message } property in a meta object provided will
automatically be concatenated to any msg already provided: For
example the below will concatenate 'world' onto 'hello':

logger.log('error', 'hello', { message: 'world' });
logger.info('hello', { message: 'world' });

This is need specifically for these use-cases:

logger.log('level', 'Any string', new Error('ok sure why'));
logger.error('Any string', new Error('ok sure why'));

The other cases are all handled by winston.formats.errors() and depend on winstonjs/logform#69.

Comment thread lib/winston/logger.js Outdated
Comment thread lib/winston/logger.js Outdated
@kibertoad
Copy link
Copy Markdown
Contributor

Also tests are failing.

@indexzero
Copy link
Copy Markdown
Member Author

@kibertoad the previous test failure was by design since this depended on winstonjs/logform#69 being merged & published (now available in logform@2.1.0).

The comment about defaultMeta is most likely a bug, but could be fixed post merge. Feel free to take it on if you'd like.

@indexzero
Copy link
Copy Markdown
Member Author

Decided to switch the order of the merge. Should address the concerns you mentioned @kibertoad.

@kibertoad
Copy link
Copy Markdown
Contributor

LGTM!

This was referenced Jan 9, 2019
@gaastonsr
Copy link
Copy Markdown

Once this is merged v3.2.0 will be published? :) can't wait.

Mizumaki pushed a commit to Mizumaki/winston that referenced this pull request Jun 11, 2020
…s). Fixes winstonjs#1338, winstonjs#1486 (winstonjs#1562)

* [tiny dist] Whitespace. package-lock.json

* [test] Add E2E integration tests with logform `errors` format.

* [test] 5:00pm. Press return.

* [fix test] More E2E errors coverage.

* [test] Test custom error properties.

* [tiny doc] Make note of duplicate coverage in `logger`. Update minor formatting.

* [test] All E2E tests work except for one...

* [fix test doc] All 14 variations of handling `new Error()` now work as most folks expect.

* [tiny] Fix up file header.

* [dist] Bump to `logform@2.1.0`

* [fix tiny] Whitespace.

* s/req_id/requestId/

* [fix test] Address PR comments. Add test coverage for defaultMeta over .child(additionalMeta)
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.

3 participants