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

Inject react-art renderer into react-devtools #13173

Merged

Conversation

@yunchancho
Copy link
Contributor

@yunchancho yunchancho commented Jul 8, 2018

Now a days, I am creating something using react-art.
However, I have suffered difficulties to debug my components based on react-art.
Because current react-art renderer isn't injected to react-devtools.
So I have made react-art be injected to devtools, and checked it works well in the browsers.

Could you review this my PR?
And let me know if I should do something for this PR to be accepted.

@pull-bot
Copy link

@pull-bot pull-bot commented Jul 8, 2018

Details of bundled changes.

Comparing: 2a2ef7e...78e092b

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +1.7% +1.9% 427.11 KB 434.44 KB 96.47 KB 98.35 KB UMD_DEV
react-art.production.min.js 🔺+2.0% 🔺+2.2% 83.25 KB 84.9 KB 25.75 KB 26.31 KB UMD_PROD
react-art.development.js +2.0% +2.3% 359.63 KB 366.96 KB 79.41 KB 81.27 KB NODE_DEV
react-art.production.min.js 🔺+3.4% 🔺+3.6% 48.23 KB 49.89 KB 15.15 KB 15.69 KB NODE_PROD
ReactART-dev.js +2.2% +2.5% 349.71 KB 357.31 KB 74.25 KB 76.14 KB FB_WWW_DEV
ReactART-prod.js 🔺+2.6% 🔺+3.6% 148.93 KB 152.82 KB 25.42 KB 26.34 KB FB_WWW_PROD

Generated by 🚫 dangerJS

Loading

@yunchancho yunchancho force-pushed the feature/react-art/inejct-devtools branch from 14b5575 to a4b4b62 Jul 9, 2018
@@ -131,6 +133,13 @@ class Text extends React.Component {
}
}

ARTRenderer.injectIntoDevTools({
findFiberByHostInstance: ReactARTComponentTree.getClosestInstanceFromNode,
Copy link
Member

@gaearon gaearon Jul 16, 2018

Choose a reason for hiding this comment

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

Does this work better than just () => null?

My impression is we only use this feature in DevTools to highlight components from DOM nodes. But ART's host instances are not DOM nodes. So we'd never make use of this.

Am I wrong? Is there any situation where ART actually does use DOM nodes (e.g. SVG mode?) In that case, can we make this code work by actually finding the fiber for an SVG node?

Loading

Copy link
Contributor Author

@yunchancho yunchancho Jul 17, 2018

Choose a reason for hiding this comment

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

@gaearon Thanks for your review. If an user sets art mode to SVG, React ART renderer creates DOM nodes for SVG. So I think that 'ref' attribute of React ART's component(e.g. Surface, Group) needs to be presented on react-devtools for debugging. The lines that you said above exists for this. In other words, that lines make react-devtools find the fiber for an DOM node of SVG.

Loading

Copy link
Member

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

I still don't understand how this works. Can you explain?

instance in your code is not going to be a DOM node. It's going to be an ART object. So your getClosestInstanceFromNode implementation will give an ART object to DevTools. DevTools doesn't know what to do with those — it expects DOM nodes.

Can you explain how this implementation is useful?

Loading

Copy link
Contributor Author

@yunchancho yunchancho Aug 2, 2018

Choose a reason for hiding this comment

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

@gaearon  I was a little misunderstanding about code line above . As you said, any instance from ReactART is not DOM node. I will remove ReactARTComponentTree.js file and change the line to findFiberByHostInstance: () => null.

Loading

@yunchancho yunchancho force-pushed the feature/react-art/inejct-devtools branch from a4b4b62 to bd1bb90 Jul 28, 2018
This commit makes react-art renderer to be injected to react-devtools,
so that component tree of the renderer is presented on debug panel of browser.
@yunchancho yunchancho force-pushed the feature/react-art/inejct-devtools branch from bd1bb90 to 5415895 Aug 2, 2018
@yunchancho
Copy link
Contributor Author

@yunchancho yunchancho commented Aug 2, 2018

@gaearon I have updated this commit to remove unnecessary code.

Loading

@gaearon gaearon merged commit b3b80a4 into facebook:master Aug 2, 2018
1 check was pending
Loading
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Inject react-art renderer into react-devtools

This commit makes react-art renderer to be injected to react-devtools,
so that component tree of the renderer is presented on debug panel of browser.

* Update ReactART.js
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants