Updates winston-graylog2 to use [email protected] #67
Conversation
Removed inapplicable tests of `processMeta` because metadata is assigned logger level, not the transport level. Also corrected invocation methods and where tests are looking for confirmation that the transport was added to winston.
|
@schfkt would you mind having a look here? |
|
@odino sure |
|
@jeremy-j-ackso sorry, I didn't have much time to check it yet due to the holidays :) I'll try to review this later today or tomorrow. |
There was a problem hiding this comment.
@jeremy-j-ackso Looks good overall! But did you try that transport yourself? Since the repo doesn't have any e2e tests now, I have no idea if this works or no.
|
@schfkt Here's a sample repo where I use it: https://github.com/jeremy-j-ackso/wgl-example. I'm not 100% sure there were e2e tests to begin with, but could be wrong. |
|
@schfkt I've updated my sample repo to include usage of a formatter to deal with errors similarly to how Also, while poking around I discovered that there's a pretty strong issue in Winston for taking care of the error formatting on the winston side of the fence. This makes me even more sure that we should just include a "suggested formatter" that covers errors, with a reference and reminder about this winston-level issue so that if/when it gets fixed or changed in winston, we're already compatible. I haven't made any modifications to address your comments, but will do that tomorrow. |
|
And apparently a possible fix for the error thing is sitting in a PR for the transport. Hopefully they'll merge it soon. |
|
@schfkt I resolved some of the comments you had, but could use some more input from you on the ones that are still out there. Also, I'm going to get those "suggested formatters" written up soon, since that seems like a good idea for people refactoring to use the updates. |
Extended the readme to include instructions for using formatters to get behavior that duplicates older version of `winston-graylog2`. Downside is that it requires using logform@^2.1.0, which hasn't been added to base winston yet. Also included a little code for including the metadata in the log output. Lastly, I noticed that the winston documentation suggests wrapping the call to the actual transport client in a setImmediate, so added that.
|
The Travis failure appears to be related to Node 6 not supporting the Rest Parameter. For it to work with Node 4 or 6, Node would have to be invoked using the It looks like the specific feature I was using that caused Node 6 to not be happy was destructuring rest parameters, which MDN (at the link) says is not supported in Node 4 or 6. |
Due to language support issues, requiring a minimum of Node 8.6.0. Also modified travis to just do `npm install`, without `--dev` option since `npm install` now defaults to installing both `dependencies` and `devDependencies`.
|
@jeremy-j-ackso Hi. Sorry, I didn't have much time last days. I'll try to look into it today or tomorrow. |
|
@jeremy-j-ackso just a side note: please don't resolve conversations for me, that way I can miss something. |
|
@schfkt All set, back to you! |
|
@jeremy-j-ackso thanks for adding e2e tests! 👍 |
|
@odino I've finished with the review. Everything looks good. One important thing: as soon as it gets merged we'll need to release another major version, since this PR has many breaking changes. |
|
We have |
Resolves #65 when using Winston 3.
Implements the transport as an ES6 class, which is more compatible with where both Node and Winston are going.
Does have breaking changes, such as removing
prelogandprocessMeta, since these situations will be better handled by Winston 3 custom formatters.