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

Do not modify Error or Date objects when logging. Fixes #610 #703

Closed
wants to merge 3 commits into from

Conversation

harriha
Copy link
Contributor

@harriha harriha commented Sep 1, 2015

Hi,

Ended up taking a stab on this issue, but not 100% sure if this is the best way to do it (and is this all) since I'm not that familiar with this lib - review welcome, luckily there's not much changes.

My concerns/notes:

  • I'm on Windows, where a bunch of tests fails locally, but I'm sure travis will tell if things are ok or not
  • Are there perf concerns here in some envs since now creating new Error/Date objects - probably not a real issue
  • Are tests in logical places and as good as they can be (not familiar with vows previously either)

The fact that these types of objects were returned as-is from clone() in the first place puzzles me a bit, wonder if there was something non-obvious behind that approach.

In case there's something to fix, please let me know.

@harriha harriha changed the title Do not modify Error objects when logging. Fixes #610 Do not modify Error or Date objects when logging. Fixes #610 Sep 30, 2015
@indexzero
Copy link
Member

Thank you for your contribution. The inner working of clone is going to change significantly in winston@3.0.0, but this may land in the 2.x branch.

if (obj instanceof Error) {
return obj;
// With potential custom Error objects, this might not be exactly correct,
// but probably close-enough for purposes of this lib.
Copy link
Member

Choose a reason for hiding this comment

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

No, this is not going to cut it for winston, sorry. Do this:

copy = new Error(obj.message);
Object.getOwnPropertyNames(obj).forEach(function (key) {
  copy[key] = obj[key];
}); 

return copy;

@indexzero
Copy link
Member

@harriha had some cycles (no pun intended) to review the Error clone approach in detail. If you can use Object.getOwnPropertyNames then I think this is good to merge.

This is the last PR I'd like to land before releasing winston@2.0.0 so it will land in the winston@1.x tree.

@indexzero
Copy link
Member

Cherry-picked this and made the change myself. Thank you for your contribution!

@harriha
Copy link
Contributor Author

harriha commented Oct 29, 2015

Hey, great to see this moving forward, thanks! And sorry for that one gotcha, didn't manage to react before you got it done, thanks for pushing this forward - and looking forward for the 2.x release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants