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] logger.info({message:...}) modifies original object #2227

Closed
wants to merge 4 commits into from

Conversation

debadutta98
Copy link
Contributor

@debadutta98 debadutta98 commented Oct 29, 2022

Hi @wbt ,

This PR closes #1775

What is wrong here?

When we copy the original object to a new object it also copies the reference of the original object to avoid this we need to implement deep copy which stores two objects in two different locations.

Work

  • Update lib/winston/create-logger.js and lib/winston/logger.js
  • Update test cases in test/unit/winston/logger.test.js

Thank you!! waiting for your response.

@debadutta98 debadutta98 changed the title [Fix] replace shadow copy with deep copy fix #1775 [Fix] logger.info({message:...}) modifies original object Oct 31, 2022
@debadutta98
Copy link
Contributor Author

hi @wbt, is this PR need any improvement

@wbt
Copy link
Contributor

wbt commented Nov 15, 2022

I don't feel like I'm the best person to review this PR, and the PR seems to include a lot of changes not necessarily related to the core of the fix you're trying to contribute (e.g. all the let to const changes, whitespace adjustments, etc.) that tend to bury the more substantive changes (like the addition of .is.not on lines 704 & 775 & 962 of the test).
There also seems to be some unnecessary repetition of new code instead of reusing a new function, which triggers those new complexity warnings, though going through to verify that sections that look repetitive are exactly repetitive and doing that refactoring is a nontrivial effort.
PRs that are large and cover multiple unrelated issues like this, especially unnecessarily so, wind up getting very low priority for the kind of in-depth review that I think is appropriate as a prerequisite for merging into such a widely used project as this. I don't anticipate having the time to do that review on this PR as it stands anytime soon. Maybe somebody else will, or you'll simplify the PR to focus on one specific issue with more DRY code that doesn't trigger new complexity warnings. Thanks for trying!

@debadutta98 debadutta98 closed this Apr 1, 2023
@bertho-zero
Copy link

Why this is closed ?

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.

logger.info({message:...}) modifies original object
3 participants