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 default value evaluation in toTyped() #575

Closed
wants to merge 2 commits into from
Closed

Conversation

luuvish
Copy link
Contributor

@luuvish luuvish commented Jul 12, 2024

Fix the issue where default values defined in the class are not properly
generated when converting Dynamic or Map objects to Typed objects.
Also, fix the issue where values are not properly generated for amended
properties when making amends to the converted Typed objects.

Cody IJ. Hwang and others added 2 commits July 12, 2024 10:21
Fix the issue where default values defined in the class are not properly
generated when converting Dynamic or Map objects to Typed objects.
Also, fix the issue where values are not properly generated for amended
properties when making amends to the converted Typed objects.
Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

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

Very sorry for letting this sit so long and thanks for the contribution. I've created an issue for this (#768).

The approach in this PR is not quite right. toTyped should really produce an object with the class default in its prototype chain (i.e. the resulting object should have the same parent as if you'd evaluated new Person { name = "NewPigeon" }). (EDIT: Hadn't thought this through; it's indeed an issue that readMember is invoked on the parent; although that doesn't change we need to revamp toTyped as per the below.)

Aside from that, the solution of introducing three readMember-like functions is costly (maintenance-wise). There is a much longer-standing issue with toTyped and that is that it is "shallow" (see issue #769). We intend to rework toTyped to cover all known issues (also issues #202 and #573).

@luuvish
Copy link
Contributor Author

luuvish commented Nov 8, 2024

Thank you for your response.

I have also been contemplating various issues related to toTyped and have been solving them in my own way.
I'm very excited to hear that you are planning to work on covering all related issues.

Regarding issue #573, const properties have the same problem. I hope all issues will be resolved cleanly.

@luuvish luuvish closed this Nov 8, 2024
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.

3 participants