-
Notifications
You must be signed in to change notification settings - Fork 49.8k
fix value formatting of proxies of class instances #30880
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
anandfresh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| return data.toString(); | ||
| case 'class_instance': | ||
| return data.constructor.name; | ||
| return Object.getPrototypeOf(data).constructor.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather change it to something like this:
let resolvedConstructorName = data.constructor.name;
if (typeof resolvedConstructorName === 'string') {
return resolvedConstructorName;
}
resolvedConstructorName = Object.getPrototypeOf(data).constructor.name;
if (typeof resolvedConstructorName === 'string') {
return resolvedConstructorName;
}
try {
return truncateForDisplay(String(data));
} catch (error) {
return 'unserializable';
}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that other stuff should be inside the try block as well, but otherwise, that looks good to me :) Do you have push access to my branch ("Allow edits by maintainers" is checked) or would you like me to commit this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please push it? You can also wrap everything in try block, yeah
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
github actions shut uppp ugh I will deal with this soon if I forgot something (I haven't yet checked if I did) |
a15f204 to
69f5040
Compare
For Hookstate Proxies of classes, `data.constructor.name` returns
`Proxy({})`, so use `Object.getPrototypeOf(data).constructor.name`
instead in that case.
69f5040 to
f724a9f
Compare
hoxyq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!

For Hookstate Proxies of class instances,
data.constructor.namereturnsProxy({}), so useObject.getPrototypeOf(data).constructor.nameinstead, which works correctly from my testing.Summary
React DevTools immediately bricks itself if you inspect any component that has a prop that is a Hookstate that wraps a class instance ... because these are proxies where
data.constructor.namereturns some un-cloneable object, butObject.getPrototypeOf(data)doesn't returnObject(it returns the prototype of the class inside).How did you test this change?
This part of the code has no associated tests at all.
Technically,
packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.jsexists, but I triedyarn testand these tests aren't even executed anymore. I can't figure it out, so whatever.If you run this code:
then
instanceProxy.constructor.namereturns some proxy that cannot be cloned, butObject.getPrototypeOf(instanceProxy).constructor.namereturns the correct value.This PR fixes the devtools to use
Object.getPrototypeOf(instanceProxy).constructor.name.I modified my local copy of devtools to use this method and it fixed the bricking that I experienced.
Related #29954