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 comments/attribute indicating which component was rendered #6559

Open
jimfb opened this Issue Apr 21, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@jimfb
Contributor

jimfb commented Apr 21, 2016

As per the discussion today...

Sometimes you are developing on a platform that doesn't have devtools (safari, etc). The problem is that you are looking at a whole pile of markup, and can't tell which components rendered it. Without devtools, the output markup is really hard to navigate. It would be cool if we had comment nodes (or a data-reactcomponent attribute) that helps users navigate the output.

These nodes would be rendered only in dev mode or with some flag turned on or something.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 22, 2016

Member

Would it have all the owners and parents too? On FB this would look like <img data-reactcomponent="Image" /> otherwise. Which is best practice so not very helpful.

This should also go in DEV mode only since the names can get minimized and removed and it's expensive putting an attribute on everything. That would be very risky since someone will start relying on the attribute being there for selectors and it will break in prod.

Member

sebmarkbage commented Apr 22, 2016

Would it have all the owners and parents too? On FB this would look like <img data-reactcomponent="Image" /> otherwise. Which is best practice so not very helpful.

This should also go in DEV mode only since the names can get minimized and removed and it's expensive putting an attribute on everything. That would be very risky since someone will start relying on the attribute being there for selectors and it will break in prod.

@mijamo

This comment has been minimized.

Show comment
Hide comment
@mijamo

mijamo Apr 22, 2016

I am more in favor of comments. This way it would be more customizable (display the Component, Key or even more info, like the nb of updates before it would be really too expensive to display the full state and props) and it avoids problems such as people thinking the attribute could be used for CSS. Comments should probably for before and after the Node.

What's more it may be only a very personnal taste but I dont like tags with huge parameters in my HTML, so I was really happy to get rid of the react-id tags in v15 and would not like to see another even bigger parameters disturb my peace in developement 😆

mijamo commented Apr 22, 2016

I am more in favor of comments. This way it would be more customizable (display the Component, Key or even more info, like the nb of updates before it would be really too expensive to display the full state and props) and it avoids problems such as people thinking the attribute could be used for CSS. Comments should probably for before and after the Node.

What's more it may be only a very personnal taste but I dont like tags with huge parameters in my HTML, so I was really happy to get rid of the react-id tags in v15 and would not like to see another even bigger parameters disturb my peace in developement 😆

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 22, 2016

Member

Comments would require the child reordering logic to take that into account. It is probably possible but I wouldn't want to make that more complicated because then it is harder to change. It is already difficult enough to come up with new algorithms.

On the flip side. It shouldn't be that hard to get the real devtools working on any browser.

The original devtools worked with Safari mobile debugging using the remote protocol. Now that we have our own protocol it should be even easier.

We could open the devtools UI in a stand alone iframe environment that inspects the main page. We could also do remote debugging using the RPC protocol. We already do this with React Native.

That would get us all the other features that we have or are building such as perf tools etc.

Member

sebmarkbage commented Apr 22, 2016

Comments would require the child reordering logic to take that into account. It is probably possible but I wouldn't want to make that more complicated because then it is harder to change. It is already difficult enough to come up with new algorithms.

On the flip side. It shouldn't be that hard to get the real devtools working on any browser.

The original devtools worked with Safari mobile debugging using the remote protocol. Now that we have our own protocol it should be even easier.

We could open the devtools UI in a stand alone iframe environment that inspects the main page. We could also do remote debugging using the RPC protocol. We already do this with React Native.

That would get us all the other features that we have or are building such as perf tools etc.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 22, 2016

Contributor

On the flip side. It shouldn't be that hard to get the real devtools working on any browser.

There are a lot of browsers in the world (including ones on dumb phones), and we can't reasonably cover all target devices/environments. Sometimes bugs only appear in specific browsers (especially older/dumber ones which don't support the devtools), which was one of the primary motivations of this conversation/issue.

Contributor

jimfb commented Apr 22, 2016

On the flip side. It shouldn't be that hard to get the real devtools working on any browser.

There are a lot of browsers in the world (including ones on dumb phones), and we can't reasonably cover all target devices/environments. Sometimes bugs only appear in specific browsers (especially older/dumber ones which don't support the devtools), which was one of the primary motivations of this conversation/issue.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Apr 22, 2016

Contributor

I don't think it would be too hard to reconcile the comment nodes, especially since we don't really walk the DOM anyway (since v15, we retain references to the nodes directly, though I suppose this might potentially change with stateless functions. Regardless, if it does change for stateless functions, then we're touching that algorithm anyway and could potentially add this).

That said, the attribute solution seems fine to me. For nodes with multiple composite parents that don't add DOM nodes, the attribute could be the parent/owner path to that element.

Anyway, I don't really have a strong preference for how we solve this, but it would be good to have a general solution that doesn't depend on us supporting the particular render target, since I doubt we want to build devtool support for all the various webviews on all the various platforms/environments.

Contributor

jimfb commented Apr 22, 2016

I don't think it would be too hard to reconcile the comment nodes, especially since we don't really walk the DOM anyway (since v15, we retain references to the nodes directly, though I suppose this might potentially change with stateless functions. Regardless, if it does change for stateless functions, then we're touching that algorithm anyway and could potentially add this).

That said, the attribute solution seems fine to me. For nodes with multiple composite parents that don't add DOM nodes, the attribute could be the parent/owner path to that element.

Anyway, I don't really have a strong preference for how we solve this, but it would be good to have a general solution that doesn't depend on us supporting the particular render target, since I doubt we want to build devtool support for all the various webviews on all the various platforms/environments.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 23, 2016

Member

We can support any browser that has a WebSocket easily. We just install the hook from the web page. There are WebSocket polyfills like socket.io that covers every browser.

You simply present the UI in a different browser than the one you're remote debugging. That's how the Safari debugger used to work when the React DevTools was first launched.

Member

sebmarkbage commented Apr 23, 2016

We can support any browser that has a WebSocket easily. We just install the hook from the web page. There are WebSocket polyfills like socket.io that covers every browser.

You simply present the UI in a different browser than the one you're remote debugging. That's how the Safari debugger used to work when the React DevTools was first launched.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 23, 2016

Member

then we're touching that algorithm anyway and could potentially add this

This is exactly what makes refactoring so hard because you can't reason about all these special cases. It is just too many of these to think about at once.

Member

sebmarkbage commented Apr 23, 2016

then we're touching that algorithm anyway and could potentially add this

This is exactly what makes refactoring so hard because you can't reason about all these special cases. It is just too many of these to think about at once.

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