Read constructor name more carefully

[LoganDark]

Summary

Sometimes constructor happens to be the name of an unrelated property, or we may be dealing with a Proxy that intercepts every read. Verify the constructor is a function before using its name, and reset the name anyway if it turns out not to be serializable.

Fixes some cases of the devtools crashing and becoming inoperable upon attempting to inspect components whose props are Hookstate States.

How did you test this change?

Installed a patched version of the extension and confirmed that it solves the problem.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 25, 2024 6:23pm

[facebook-github-bot]

Hi @LoganDark!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[hoxyq]

Thanks for upstreaming this, I have a few questions:

• Is it possible to add a test which shows that React DevTools is failing to inspect some of such components?
• Why do we need to use structuredClone and not just typeof <name> === 'string'?

[LoganDark]

• Is it possible to add a test which shows that React DevTools is failing to inspect some of such components?

I'm not sure. I don't really have much time to do that right now, though. I don't imagine it'd be hard, just pass a prop that is {constructor: {name: () => {}}}.

Why do we need to use structuredClone and not just typeof <name> === 'string'?

AFAIK it could be a proxy pretending to be string. It's not really the type that we need to check here, it's whether trying to send it over the bridge would throw exceptions. structuredClone is a decent way to check because it should throw errors in the same situations.

Of course we could probably put a try/catch around each attempt to send a message as well so that unserializable messages don't permanently brick the queue.

[hoxyq]

• Is it possible to add a test which shows that React DevTools is failing to inspect some of such components?

I'm not sure. I don't really have much time to do that right now, though. I don't imagine it'd be hard, just pass a prop that is {constructor: {name: () => {}}}.

Why do we need to use structuredClone and not just typeof <name> === 'string'?

AFAIK it could be a proxy pretending to be string. It's not really the type that we need to check here, it's whether trying to send it over the bridge would throw exceptions. structuredClone is a decent way to check because it should throw errors in the same situations.

Of course we could probably put a try/catch around each attempt to send a message as well so that unserializable messages don't permanently brick the queue.

I think the change makes sense overall, but I would prefer us to be more conservative about the the constructor.name, this should be a string. Bridge is agnostic to the way the data is transferred, it could be anything: browser extension API, websocket message, Chrome DevTools protocol Runtime.bindingCalled event.

AFAIK it could be a proxy pretending to be string.

I am not sure if you can actually implement a proxy that would override typeof in a way that it would return string. Anyways, this seems like an extreme case we shouldn't explicitly cover.

Would you be able to update the PR accordingly?

[LoganDark]

Would you be able to update the PR accordingly?

You mean to just check if the type is string instead of trying structuredClone?

[hoxyq]

Would you be able to update the PR accordingly?

You mean to just check if the type is string instead of trying structuredClone?

Yes.

[hoxyq]

LGTM. Thanks for fixing this.

Please fix the linter issues, and then we can merge this.

[LoganDark]

Ugh, it's another format checker that doesn't provide a diff. (Actually, I think it's always been prettier that consistently does not.)

I suppose I can do that next Monday when I have access to a terminal. I could've done it now if I had a diff, but if I have to clone the repo to my local machine and run a command on it, I can't do that until next Monday since I'm off this week.

I can't even imagine what prettier might be getting upset about anyway.

[LoganDark]

yeah okay, I truly have no clue what Prettier is getting upset about. I guess I'll properly run the command next Monday. And maybe then I'll open another PR to fix your CI (or open a PR to Prettier to fix their project), as it's really supposed to be providing a diff.

(apologies if this comes off as rude, I mean nothing by it. just frustrated by this having happened multiple times, i.e. having to clone a repo to my computer just to run the stupid formatter because the CI complains that something is wrong without telling me what.)

(and update: looks like it is an issue with Prettier itself, go figure. Not your fault then.)

[LoganDark]

thanks!