Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Detect duplicated React #714

Closed
wants to merge 5 commits into from

Conversation

chisler
Copy link

@chisler chisler commented May 12, 2017

Resolves #700.

Most popular renderers that affect chrome, except one, do not inject themselves in DevTools hook as I stated in this comment.

We want to:
Detect multiple copies of ReactDOM

On renderer injection we:

  1. Try to filter out all renderers other than ReactDOM.
  2. Count injected renderers.
  3. More than 1 => report duplication.

Filtering relies on specifics of production builds of ReactDOM Stack v15.*, v0.14.* and v0.13.* and Fiber.


Issues:

  1. This filtering is not perfect, it's a way to avoid changing react-dom, so if you see a better way, it's cool.
  2. Current version of DevTools overwrites build type reported with each consequent renderer injection. Seems like it's better to report all the builds of renderers to cover "valid multiple renderers" case in future.
  3. Function isReactDOM() works similar to detectReactBuildType() => mirrors it's need for higher fault tolerance.

Examples:

| Detected |
Filtered out |
|---|---|
| image | image |
| Website uses two copies of React: from vendors.js and chatlio.min.80ec1f7e.js. | Website uses two renderers: unminified ReactDOM and unminified ReactThreeRenderer (which is filtered out) |
| Result: duplicate reported. | Result: unfinified reported. |

@chisler chisler changed the title Detect duplicated react Detect duplicated React May 12, 2017
@chisler
Copy link
Author

chisler commented Aug 3, 2017

Hey guys,

It's been 3 months since I posted it.

I respect OSS, I'm just wondering if I did something wrong. I'd like to fix this and update the branch for merging.

I mention @gaearon, the author of issue #700.

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2017

Hey, I’m sorry we’re not actively reviewing PRs on this repo (or anywhere, really). We’re in churn mode trying to ship React 16 (announcement, remaining work to do). We’ll get back to reviewing stuff once 16 is shipped.

That said this PR looks really great. But I think we should allow multiple renderers to inject themselves. What do you think about adding a string to inject() call in React that would pass id: 'react-dom' or something like this? Then we could deduplicate them correctly.

@chisler
Copy link
Author

chisler commented Aug 3, 2017

May be the best possible solution, because it minimizes code-guessing:

  1. Since now inject react-dom renderer passing the id argument.
  2. Try to detect old versions of react by code, as I do now.

I'll be back with this.

Thanks for reaching out! Good luck with shipping the 16.
And I'd love to help if possible after this one is finished.

@gaearon
Copy link
Contributor

gaearon commented Aug 3, 2017

Happy to take PRs to React repo adding id to the inject call.

@chisler chisler force-pushed the detect-duplicated-react branch from 22bb668 to 5d06481 Compare August 17, 2017 10:29
@chisler
Copy link
Author

chisler commented Aug 17, 2017

Hey! I made the PR to React and updated the branch here.

Tests that fail on my branch with

.../babel-preset-es2015/index.js" provided an invalid property of "name"

fail on master as well, at least on my computer.

@gaearon
Copy link
Contributor

gaearon commented Aug 17, 2017

Can you figure out why it is failing?

@chisler
Copy link
Author

chisler commented Aug 17, 2017

Yeah, I'd love to. Will do it in few days.

@chisler
Copy link
Author

chisler commented Aug 31, 2017

The errors were provided by 3rd party packages. After reinstalling all the packages in two weeks tests are alright:)

@gaearon
Copy link
Contributor

gaearon commented Sep 13, 2017

I think the last remaining blocker for me is facebook/create-react-app#3120.

@chisler
Copy link
Author

chisler commented Sep 21, 2017

Hi. Thanks for the comment and attention, sorry for the delay. I'm glad @tharakawj is already working on this. I'll look into ReactART's injection.

# Conflicts:
#	backend/installGlobalHook.js
@chisler
Copy link
Author

chisler commented Oct 19, 2017

Small note on merging master

Since we now have deadcode build type I prioritised duplicated over deadcode because duplication describes the app as a whole, deadcode — one specific renderer.

I assume it's the best solution before we could report table of all the checks about each renderer in popup.

@bvaughn
Copy link
Contributor

bvaughn commented Aug 20, 2019

React DevTools has been rewritten and recently launched a new version 4 UI 🎉 The source code for this rewrite was done in a separate repository and is being merged into the main React repo.

Thank you for taking the time to contribute to this extension 🙇‍♂️

Because this PR is for the version code base, I am closing it. If you'd like to contribute to the new extension, please visit the new repository.

@bvaughn bvaughn closed this Aug 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect duplicate React and show a different message
5 participants