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

Add isArray in devTools utils #22438

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

behnammodi
Copy link
Contributor

Also here I replaced Array.isArray with isArray from 'shared' package

@bvaughn
Copy link
Contributor

bvaughn commented Sep 27, 2021

When Sebastian landed this in #21188, he confirmed that the Closure compiler inlines the wrapper function. Unfortunately DevTools doesn't use the same compilation process, and as a result, the wrapper/indirection doesn't get inlined.

Maybe longer term, we should look into running Closure compiler over DevTools builds as well– but in the short term, I'm not sure the added benefits from this change are worth the extra wrapper.

Maybe we could use a similar approach as shared/hasOwnProperty for DevTools (e.g. react-devtools-shared/src/isArray) and just export the function directly?

@behnammodi
Copy link
Contributor Author

Yes, I agree, I will do it

@behnammodi
Copy link
Contributor Author

@bvaughn Done

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

👍🏼 Thanks

@bvaughn
Copy link
Contributor

bvaughn commented Sep 27, 2021

I'm not sure why CI isn't properly running on your PR. Each time you push, it looks like only the CLA check and Code Sandbox run.

@behnammodi
Copy link
Contributor Author

I'm not sure why CI isn't properly running on your PR. Each time you push, it looks like only the CLA check and Code Sandbox run.

Yes, this is exactly my question, maybe CI knows me :)

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

It's fine. This change seems safe, and I ran the tests locally on your branch. Not sure why CI is not cooperating.

@bvaughn bvaughn merged commit 580e2f5 into facebook:main Sep 27, 2021
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants