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

DevTools: Fully disable 0.14 support #16462

Closed
bvaughn opened this issue Aug 19, 2019 · 10 comments · Fixed by #16897
Closed

DevTools: Fully disable 0.14 support #16462

bvaughn opened this issue Aug 19, 2019 · 10 comments · Fixed by #16897

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Aug 19, 2019

It’s confusing that 0.14 or earlier is in half-working state where it displays a tree (but incorrectly). We should detect it and fully disable if it doesn’t work. Or fix it.

If we go the route of disabling support:

  1. DevTools should show a warning message that clearly indicates the version of React isn't supported. (This is probably a good idea for v13 and older anyway.)
  2. DevTools should not throw any errors.

Originally reported by @gaearon via bvaughn/react-devtools-experimental#384

@danielhusar
Copy link

I can take a look if nobody started on this one yet :)

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 6, 2019

It's all yours @danielhusar. Please let me know if you decide not to work on it for any reason, and I'll remove the "taken" label 😄

@danielhusar
Copy link

@bvaughn Hey! Is there an easy way to get react version?
I'm looking into window.__REACT_DEVTOOLS_GLOBAL_HOOK__.renderers but looks like version has been only added in recent react versions. Or perhaps perform some feature detection?

It's also possible I'm looking into a totally wrong place :)

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 9, 2019

I think you would have to feature detect it. Here's how the legacy extension did it:
https://github.com/facebook/react-devtools/blob/d839081f79f681617df7584a8ffd2f4163fa40b9/backend/attachRenderer.js#L24-L76

Based on this, I think checking for renderer.ComponentTree should be a sufficient way of differentiating between v15 and older. (But you'd want to double check me by testing it.)

@danielhusar
Copy link

@bvaughn I have very early PR #16720 , would you be able to check if I'm remotely going in right direction?

It's my first time in codebase and I'm bit struggling. I think I have the check for the extension there, but I have hardcoded values for standalone | frontend, as I haven't tracked down where to put those checks yet.

I was looking into adding some logic also inside onBridgeOperations in the store, but operations argument is just array of numbers, which I'm not sure have all the info for this.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 10, 2019

I was looking into adding some logic also inside onBridgeOperations in the store, but operations argument is just array of numbers, which I'm not sure have all the info for this.

I've written some documentation about how the operations array works here, if it's helpful:
https://github.com/facebook/react/blob/master/packages/react-devtools/OVERVIEW.md#serializing-the-tree

I left a few comments on the PR.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 25, 2019

Hi @danielhusar! It's been a couple of weeks since the PR has been updated, so I'm going to remove the "taken" label for now. Let me know if you pick this back up though!

@danielhusar
Copy link

Sorry, been pretty busy with deadlines lately :(

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 25, 2019

It's fine :) Just wanted to free it up for someone else (maybe even me) to take a look.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 26, 2019

This fix has just been published to NPM and posted to Chrome/Firefox as v4.1.1

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.

2 participants