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

Fix: log Errors on winston ^2.3.1 #13

Closed
wants to merge 4 commits into from
Closed

Conversation

rodrigo4244
Copy link
Member

@rodrigo4244 rodrigo4244 commented Jan 25, 2018

Description

This PR fixes the issue winstonjs/winston#1178 from winston using a proxy on our logger.

Test plan

  1. Create a temporary package dir (mkdir /tmp/x;cd /tmp/x;npm init --yes)
  2. Add logger (yarn add invisible-tech/logger#rgo/fix-winston)
  3. Create repro.js with const logger = require('@invisible/logger');logger.error(Error('Errors can be cool'))
  4. Run node repro.js

The output is an error object "equals" to the one returned on [email protected].
It is not only the error message as on the card: https://trello.com/c/9p0K6DFe/776-logging-error-objects

@@ -0,0 +1,43 @@
'use strict'

const LOGGING_LEVELS = [
Copy link
Member Author

@rodrigo4244 rodrigo4244 Jan 26, 2018

Choose a reason for hiding this comment

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

I copied LOGGING_LEVELS from src/helpers/assertLevel.js because this file is going to be deprecated when the bug is solved on winston.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make the above a code comment

index.js Outdated
transports,
})

const logger = cloneErrorProxy(unwrappedLogger)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment in this file as well as to why we're doing that

'silly',
]

// FIX: winston from 2.3.1 until 2.4.0 doesn't log Error objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

did this comment miss the word "because" somewhere? i don't fully understand it.

@gunar
Copy link
Contributor

gunar commented Jan 26, 2018

clean approach! i just want two more comments ;)

after winston solves its bug this PR gets deprecated
index.js Outdated
@@ -14,6 +14,9 @@ const unwrappedLogger = new (winston.Logger)({
transports,
})

// We are proxying winston logger because versions from 2.3.0 until 2.4.0
// does not log Error objects. The proxy is going to be DEPRECATED
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: does -> do
but ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :)

@gunar
Copy link
Contributor

gunar commented Jan 29, 2018

Should we have used this instead?
https://github.com/ds300/patch-package

@rodrigo4244
Copy link
Member Author

This is pcool man! It will be an improvement to our system, we could even add the retryConfig to Slack's WebClient!

Add a new card to me and close this PR if you want me to patch winstonjs.
Some people already made the changes there, we just need to copy/paste and run the patch.

@gunar gunar assigned kewitz and unassigned gunar Jan 29, 2018
@gunar
Copy link
Contributor

gunar commented Jan 29, 2018

@rodrigo4244 @kewitz you make a decision on what is best and report (share in the meeting)

@rodrigo4244
Copy link
Member Author

This is deprecated because of #14

The code is archived on the following tag: https://github.com/invisible-tech/logger/releases/tag/archive%2FcloneErrorProxy

@rodrigo4244 rodrigo4244 deleted the rgo/fix-winston branch January 30, 2018 08:52
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