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
Don't modify arguments passed to logging methods #1549
Comments
Please fix so that data is not unexpectedly mutated. It looks like there was some confusion between the Triple-beam and non-enumerable properties, wherein the |
Avoiding an extra copy for a logged object can be a significant performance gain, so I believe that's the reason winston works with the original object rather than a copy. We're aware of what you mentioned with non-enumerability and triple-beam properties, but other users would also be unhappy even if all the properties we set were non-enumerable :) Perhaps a good solution is to add an option (default false) to Logger that specifies copying objects rather than using them directly (which in turn mutates them). @indexzero open to your thoughts on this. Another relevant issue that's open #1486 is what to do with an object you log that contains a field like |
Yes @DABH, the only thing I have mentioned is mutating the object at all would be bad practice. And I don't think copying the object is any less evil. The ideal way to manage objects, in Javascript, is through context and inheritance and the evolution used by a lot of other modules is that they drop their signature patterns altogether and end up with a single object parameter, so compared to:
We'd instead have all options as:
Or if we want to keep the signature pattern, then we can simply do this on the receiving end:
|
Yeah, I would definitely not start deep copying arguments. It seems to me like Now, I could override something like that to have mutative side effects, but my point here is not that the library should bend over backwards to control for that, just that it shouldn't do it itself. |
This is by design. The README for winston.log({ level: 'warn', ...err }); @sparkida unfortunately there is no easy way to handle all permutations of possible inputs that folks which to provide. In particular the desire of most developers includes:
... and many others. In general from a historical perspective, There are trade-offs for performance vs. features. We opt for performance whenever possible. |
Thanks @indexzero, I didn't know that would avoid mutation. |
The log utility we use can't have side effects. This is a deal-breaker for us, unfortunately. |
@dpurrington pino is a great logger, I highly recommend it. |
Please tell us about your environment:
winston
version?winston@2
winston@3
node -v
outputs: v11.4.0What is the problem?
Winston modifies objects passed to it!
I have a webservice with error handler:
I expect, say, a 404 response with body
{"message":"Not Found"}
, but with thewinston.warn
call included, it becomes{"message":"Not Found","level":"warn"}
. Of course, it's worse after going through formatters—I get tabs, ANSI color codes, etc.—but out of the box I still get thelevel
property.What do you expect to happen instead?
No mutation
Other information
The text was updated successfully, but these errors were encountered: