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

Serialization also calls inherited toJSON #1732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nechaev-chromium
Copy link

@nechaev-chromium nechaev-chromium commented Apr 5, 2023

Serialization mechanism calls not only own toJSON method but also inherited toJSON method of the serialized object

Fixing Call of toJSON in the internal JSON clone algorithm.


Preview | Diff

@nechaev-chromium nechaev-chromium changed the title Serialization also calls inherited toJSON (#1730) Serialization also calls inherited toJSON Apr 5, 2023
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Generally it looks fine to me but I wonder what might be missing in case an object inherits toJSON() and defines some own properties. In case handling only an own toJSON() call, we could assume that the object knows what to serialize, but if we call it on one of the super classes it might miss values?

@jgraham what do you think?

@@ -6581,8 +6581,8 @@ <h3>Executing Script</h3>
<li><p>Return <a>success</a> with data <var>reference</var>.
</ol>

<dt>has an <a>own property</a> named "<code>toJSON</code>" that is
a <a>Function</a>
<dt>has an <a>own property</a> or <a>inherited property</a> named
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<dt>has an <a>own property</a> or <a>inherited property</a> named
<dt>has a property</a> named

Given that it doesn't matter if it's an own or inherited property we potentially could just use this term?

@whimboo whimboo assigned whimboo and jgraham and unassigned whimboo May 26, 2023
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.

4 participants