add modifiers to debug render tree

[patricklx]

No description provided.

[patricklx]

prints error stack traces from tests to console, useful for debugging, like clicking on error location to jump to the code

[chancancode]

I am not sure that it is a good idea to add more work here, especially to the close element operation which affects every single element we render which can add up. I don’t love the architecture of the inspector integration today, the code is not strippable, has some non-zero impact even when it’s not enabled, and IIRC it’s enabled pretty much any time that the inspector is installed, not just when the inspector is visible or the rendering tab is selected. We probably need to think harder about whether that is ok before adding even more work there, especially something as impactful as this.

Doing it for modifiers itself is probably not an issue, it’s the fact that you need some way to tie it to the element and that led you to add HTML nodes to the tree that is the problem. Beyond what I said above it will also make the tree vastly bigger and you may run into slowness in serializing the tree and sending it over to the inspector. That happens every time something changes and causes a re-render (even for no-op re-render).

If you just want to add modifier node without introducing html element parents here I think that would be fine. We still give you a reference to the element anyway and you can probably still group them in the inspector’s processing code if you want.

[patricklx]

@chancancode i tried to improve the performance by checking with a Set.
It actually only creates the html element if there are modifiers.
I also wanted components that are under the html element to actually appear under the element.
If this is still not okay, i could also just create the html element in the pushModifiers call and close it immediately afterwards. That shouldn't cause much more perf issues

[chancancode]

Maybe you are not fully done, but to be clear about what I meant in the comment – in the final state of the PR (w.r.t. "I think it would be fine if we just want to add modifiers to the tree without adding elements"), this should not even be needed, and that we wouldn't have added the new "html-element" type to the tree at all. When I say you can infer and group them in the inspector if you want to, I mean you can do that using the bounds, which would presumably reference the element they are attached to, and then you can recreate the grouping with a weak map or something on the inspector side.

And, yes, if it's feasible and about the same amount of code, it would probably be a good idea to do the work in pushModifier rather than in closeElement, all else being equal.

[patricklx]

so, now there is a html element which wraps only the modifiers. the rest can be done in inspector

[chancancode]

As I was saying above, let’s not even add that

[patricklx]

updated

[chancancode]

see above

[patricklx]

@chancancode i update the PR.
by the way, inspector does not serialize+send every time something changes. I did improve the inspector long time ago to limit the calls to debug render tree (and serialize+send) to once per 250ms.
Maybe if someone has many items of components+modifiers it starts slowing down . But I also have an idea for that, at least at inspector side.

[chancancode]

Gonna circle back to the inspector stuff after finishing #1568 (which could use a review from you)

[patricklx]

I found that in-element needed to add a destructor to release it in the debug render tree. Does this also need something similar?

[patricklx]

I think i move everything to

glimmer-vm/packages/@glimmer/manager/lib/public/modifier.ts

Line 68 in 17ca94f

export class CustomModifierManager<O extends Owner, ModifierInstance>

[chancancode]

Most of these are analogous to the component equivalent in the same file

[NullVoxPopuli]

Thanks for doing putting this together! and the test was most helpful in helping understand what's expected of the debug render tree for modifiers <3