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 renderer id to react-devtools injection #10475

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

chisler
Copy link
Contributor

@chisler chisler commented Aug 17, 2017

Helps to resolve react-devtools 700 — reporting multiple react-dom copies.

I added id argument to detect react-dom without code-guessing.

@gaearon
Copy link
Collaborator

gaearon commented Aug 17, 2017

I think this is good, thanks.

@sebmarkbage
Copy link
Collaborator

@gaearon Should we bikeshed the name a bit since we have to live with it forever?

@@ -813,6 +813,7 @@ const foundDevTools = injectInternals({
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
id: 'react-dom',
Copy link
Collaborator

Choose a reason for hiding this comment

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

rendererType: 'react-dom'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

rendererName? rendererUniqueName? People cloning ReactDOM might think a "type" would still be ReactDOM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

rendererPackageName? As in the npm name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Let's change to rendererPackageName.

@chisler
Copy link
Contributor Author

chisler commented Aug 21, 2017

Hey! I changed the name.

@@ -813,7 +813,7 @@ const foundDevTools = injectInternals({
// This is an enum because we may add more (e.g. profiler build)
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
id: 'react-dom',
rendererPackageName: 'react-dom',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also please add this to React Native entries?

@chisler
Copy link
Contributor Author

chisler commented Aug 21, 2017

It seems like we don't want to see any renderer duplicated, not react-dom only. But that might just not exist in the real world.

About adding to react-art/....

@gaearon
Copy link
Collaborator

gaearon commented Aug 21, 2017

Can you file another PR for ReactART? I think it doesn't even contain a call now. We should add it.

@gaearon gaearon merged commit ec77740 into facebook:master Aug 21, 2017
@chisler
Copy link
Contributor Author

chisler commented Aug 21, 2017

I'll do that. I'll update the devtools PR as well, detecting not react-dom only, but two duplicated renderers that provided id or were detected by code.

@gaearon
Copy link
Collaborator

gaearon commented Aug 21, 2017

That sounds great. Thanks!

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.

4 participants