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

Properly handling the 'nestedKey' option #883

Closed
mihai1voicescu opened this issue Aug 4, 2020 · 3 comments · Fixed by #885
Closed

Properly handling the 'nestedKey' option #883

mihai1voicescu opened this issue Aug 4, 2020 · 3 comments · Fixed by #885

Comments

@mihai1voicescu
Copy link
Contributor

Let me start by congratulating you guys on this awesome project and by saying I have read #872, #771 and #792 and the docs available here.

Personally what I understand from the docs (and what I would expect to happen) is that the serializers will still receive the original object, not the one after the 'nestedKey' transformation (as the base is static, the dynamic part is the passed object and the serializers register only on root level properties, making the serializers register feature pointless when used with 'nestedKey'). But this is my opinion and is subject to interpretation.

However, activating the 'nestedKey' option breaks the standard serialization of errors (because the detection fails) which is a bug (standard options should not break each other).

The problem seems to be that the passed object is transformed here and afterwards is checked for it's type here effectively transforming all Errors into plain objects so the detection is never true.

I think it would be a good idea to do the transformation after the check.

If you guys agree I would be happy to make a PR.

@mcollina
Copy link
Member

mcollina commented Aug 4, 2020

Sure, go ahead with the PR.

mihai1voicescu pushed a commit to mihai1voicescu/pino that referenced this issue Aug 4, 2020
…error detection and serialization (closes pinojs#883)

proto.js
* Copy the message property in case the obj is an error and there is a msg specified
* Load the msg as a property of obj if metadata is needed

symbols.js
* Add nestedKeyStrSym to quickly cache the string

tools.js
* Manually build the stringify for performance reasons (not too pretty...)

error.test.js + serializers.test.js
* Add a test for the new nestedKey interaction
mcollina pushed a commit that referenced this issue May 19, 2021
#885)

* Make the nestedKey only take effect in the serialized object and fix error detection and serialization (closes #883)

proto.js
* Copy the message property in case the obj is an error and there is a msg specified
* Load the msg as a property of obj if metadata is needed

symbols.js
* Add nestedKeyStrSym to quickly cache the string

tools.js
* Manually build the stringify for performance reasons (not too pretty...)

error.test.js + serializers.test.js
* Add a test for the new nestedKey interaction

* Add tests to complete the coverage

* Address PR notes

* Fix test title

* Fix tests

Co-authored-by: Mihai Voicescu <[email protected]>
@mihai1voicescu
Copy link
Contributor Author

Solved in #885

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants