Don't render empty unnecessary component closing comments

[mondeja]

I'm seeing a lot of empty comments in the HTML of a webapp deployed on production.

Looking at the code I suspect that the closing comment of ComponentRepr should not be included in release builds, avoiding a lot of potential calls to insert nodes in the DOM. Are these comments neccessary for some reason?

[gbj]

Just so I understand the question and can answer more helpfully: What specifically are you concerned about here? Performance? Styling? Memory use? A user opening up the DOM inspector and seeing a bunch of comments? Something else?

There are probably ways we can reduce this but it's likely removing the Component wrapper from around component function bodies in the #[component] macro in release mode, which has some effects on debugging/tracing and would need to be tested to make sure there aren't other rendering issues caused by that.

Generally speaking in release mode there's a closing comment which marks the boundary between two components; this is necessary for things like dynamic children, fragments, and <For/>, which may need to move some arbitrary number of elements in the DOM. It's possible it's not necessary for other components, but comment nodes are very cheap.

Your image suggests to me you have a bunch of nested components that don't actually create HTML elements, but maybe I'm wrong.

[mondeja]

Just so I understand the question and can answer more helpfully: What specifically are you concerned about here? Performance? Styling? Memory use? A user opening up the DOM inspector and seeing a bunch of comments?

Thanks for the detailed explanation. A bit of everything, but I'm not really concerned about that, I suppose that is useful for components like For, is understandable.

Your image suggests to me you have a bunch of nested components that don't actually create HTML elements, but maybe I'm wrong.

Some Metas, some Links, a Title, a Router with Routes and Route... etc. There are so many components that are not designed to render anything, hence my question.

[gbj]

Your image suggests to me you have a bunch of nested components that don't actually create HTML elements, but maybe I'm wrong.

Some Metas, some Links, a Title, a Router with Routes and Route... etc. There are so many components that are not designed to render anything, hence my question.

Yeah this all seems completely reasonable. I'll have to think about it a little more but I'm pretty sure we should be able to remove the redundant ones. Thanks for bringing this up.

[gbj]

Small update: This has been done in my early work on a new renderer for 0.6, which does not insert any marker nodes for types that return (), and only uses markers at all for types that genuinely need them.

[reedwoodruff]

As an addition to this discussion, I have a use case where I'm dependent on a consistent ordering of child elements in the DOM.
The filler comments can sometimes play havoc with this, and result in different behavior between trunk serve --release and normal trunk serve.
Release:

Normal:

So when I'm developing in normal mode, I'd need to target the second child element of the div in order to get the text node, whereas in release it's the first child node.

A while back I tried to look through the source code to understand how/why the comments were generated, but wasn't able to really understand. Do you think this case would be addressed in the 0.6 changes?

Bonus pain point around all of the comments: it makes the element inspector in the browser's dev tools very slow when you have a significant number of components (up to 1000), each producing ~30 comments.

[gbj]

@reedwoodruff contenteditable in general is very difficult to make work correctly with any framework. I'd go as far as to say that if you are using contenteditable on an element, you should assume that element should be a black box from the framework's perspective: initial content can be provided via inner_html and the results contents can be manipulated manually, but the framework will not be able to accurately keep track of/update the contents of any contenteditable element.

In 0.6, we can

• remove lots of markers for e.g., () types (fine)
• remove component boundary markers (there's some DX loss here in being able to use the inspector to see the component tree as well, but I guess eventually we can build some real devtools)

We cannot remove every marking comment, because they are needed as placeholders for a certain set of dynamic elements unless me massively restrict the flexibility of the framework.

[reedwoodruff]

@reedwoodruff contenteditable in general is very difficult to make work correctly with any framework. I'd go as far as to say that if you are using contenteditable on an element, you should assume that element should be a black box from the framework's perspective: initial content can be provided via inner_html and the results contents can be manipulated manually, but the framework will not be able to accurately keep track of/update the contents of any contenteditable element.

In 0.6, we can

• remove lots of markers for e.g., () types (fine)
• remove component boundary markers (there's some DX loss here in being able to use the inspector to see the component tree as well, but I guess eventually we can build some real devtools)

We cannot remove every marking comment, because they are needed as placeholders for a certain set of dynamic elements unless me massively restrict the flexibility of the framework.

Fair enough, I know that contenteditable is kind of the wild west. In particular I'm more worried about the difference in comment structure between --release and other profiles. It seems like this could cause issues in other cases besides contenteditable -- anywhere a developer is trying to walk the DOM tree to find particular nodes and relying on a consistent ordering (i.e. anywhere manual dom interaction is happening and the developer expects the node added to be the first node, but sometimes it's a comment node).

[rakshith-ravi]

#972 would fix a lot of such issues, wouldn't it @gbj?

[gbj]

#972 would fix a lot of such issues, wouldn't it @gbj?

I guess I'll answer "sort of, but not really," and the same changes that close that will close this. On the specific question about sibling nodes/walking the DOM, I'd say assuming that there will never be any comment/placeholder nodes used by the framework will not be accurate, but the number of them will be greatly reduced, and the use of hydration IDs on elements (#972) will not be necessary any more.

[gbj]

Done in 0.7.