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

Redundant condition in react-devtools-shared #20905

Closed
nlspnsgen opened this issue Mar 1, 2021 · 4 comments · Fixed by #21101
Closed

Redundant condition in react-devtools-shared #20905

nlspnsgen opened this issue Mar 1, 2021 · 4 comments · Fixed by #21101

Comments

@nlspnsgen
Copy link

nlspnsgen commented Mar 1, 2021

This is not important but it seems that this if statement is superfluous since typeof always returns a string.

if (typeof data[Symbol.iterator]) {

@bvaughn
Copy link
Contributor

bvaughn commented Mar 1, 2021

Yeah, looks like maybe this got broken during a refactor (ce65df7).

I think this line should be

if (typeof data[Symbol.iterator] === 'function') {

Actually since we check the type right above, maybe the condition should have just been deleted entirely. (I don't remember this code very well.)

@E-Aho
Copy link
Contributor

E-Aho commented Mar 25, 2021

Hey, I'm gonna pick this up and try to get a PR through to tidy this :)

This would be my first issue in this repo. I've given the contribution guidelines a read through, and so will do my best to follow all the CoC and best practices in there.

@bvaughn
Copy link
Contributor

bvaughn commented Mar 25, 2021

This issue is all yours @erikaho! 😄

I've added the "good first issue (taken)" label so that others will know not to start work on the issue. If you change your mind about the issue, no worries! Just let me know so that I can remove the label and free it up for someone else to claim.

Cheers!

@E-Aho
Copy link
Contributor

E-Aho commented Mar 25, 2021

Hey! Raised a PR for this.

Couldn't find any unit tests that covered the dehydrate function here, and I started to write a few, but thought that it was probably out of scope of this PR. Can definitely create some tests for this function if needed though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants