add in element to debug render tree

[patricklx]

My learnings from hacking on this in inspector
emberjs/ember-inspector#2549

Adding modifiers is also easy, will add it in another pr

[NullVoxPopuli]

Looks good to me.

Let's give @chancancode or @wycats a chance to review tho (couple days?)

[chancancode]

Suggested change

	this.env.debugRenderTree.create(state, {
	type: 'remote-element',
	name: 'in-element',
	args: {
	positional: [],
	named: {
	destination: ref,
	},
	} as any,
	instance: {
	args: {
	destination: ref,
	},
	constructor: {
	name: 'InElement',
	},
	},
	});
	this.env.debugRenderTree.create(state, {
	type: 'keyword',
	name: 'in-element',
	args: createCapturedArgs(
	// not sure why the order is backwards, but it's named then positional
	{ insertBefore: createConstRef(insertBefore, false) },
	[createConstRef(element, false)],
	),
	instance: null,
	});

[chancancode]

*see the other comment regarding state

[patricklx]

i wasn't able to use createCapturedArgs, because importing ./arguments would cause recursive imports

[chancancode]

Hm, let's see how we can resolve that, what was the cycle?

[patricklx]

I'm not entirely sure

I think

• arguments
• stack
• low-level
• opcodes
• element-builder

But i also saw this:

• arguments
• compiled/opcodes/-debug-strip
• ../vm/arguments

[patricklx]

The error happens in glimmer/runtime/lib/compiled/opcodes/vm.ts:38:1
APPEND_OPCODES is not defined

[chancancode]

I'll take a look when I get a chance 👌🏼

[chancancode]

this.popBlock() returns the bounds so you would just call it first, store it in a local variable, and then pass that to didRender, rather than having to make your own

[chancancode]

As for "state", it just have to be a stable object that uniquely identify this node, and so it actually works to just use the "block" object – the one you get back from calling pushLiveBlock and popBlock (regarding my comment above, Block implements Bounds), which would be the same object, because that is already stored in a stack, which is why I said it would be unnecessary for you to maintain yet another parallel stack just to track the same.

However, it looks like something is currently wrong with the types so you would have to fix it before you can do that.

First of all, you should have access to the block returned by pushLiveBlock (so you can pass it to debugRenderTree.create from within pushRemoteElement). However, that is happening from with __pushRemoteElement, which currently has a return type of Nullable<RemoteLiveBlock>. I think that is wrong – specifically, the Nullable<...> is not accurate, as far as I can tell, there is no code path that would return null, and if there is, it would be a bug to unconditionally call popBlock here in popRemoteElement.

So you'll have to fix that return type, and then do the same in the rehydration builder, then, you should have everything you need to implement the feature without having to add your own parallel stack.

[chancancode]

And – while you are in this file, we really shouldn't be inferring the return types of these methods in this file (ie. a bunch of methods does not have : ... here). It seems like most of them are just : void and we should have typed that explicitly.

Ideally we should fix that in a separate PR, either just in this file, or across the whole codebase and enforce it with a lint, that would be great. It will probably give @NullVoxPopuli some rebase nightmares though, so you may want to coordinate with them on Discord about how to approach it; I think it is ultimately a rather mechanical fix that should be possible to do, but @NullVoxPopuli can figure that out with you.

[patricklx]

block seems to have the bounds set to the children of the remote element. so I reverted to pass custom bounds

[chancancode]

That is the correct usage of bounds though. It is what each node has rendered. A component’s bounds is its content. And you need that if you want to draw the bounding box if the inspector

[patricklx]

True, it will not work with insertAfter, otherwise this would work.
Would it be okay to still have custom bounds where parentElement is the parent of the in-element, like how it is now? I would like to have that link for the inspector.
first&last would use the block bounds.
From the code in glimmer i think parent element is checked to verify if a node was removed or moved to another block.
In the inspector it was only used to check if a node is still in the dom. It uses node.isConnected now.

[chancancode]

@patricklx I don't really understand this, but no, something sounds off. The bounds represents a range in the DOM, denoted by (element, firstChild, lastChild) where first/last child belongs to the element. You should expect to be able to find a sub-slice in element.childNodes that matches (firstChild, lastChild). The inspector should be using this to highlight (bounding rect overlay). So something seems fishy about what you wanted, can you link to the code?

[patricklx]

yes, first and last child is the same .
And inspector only uses those:
https://github.com/emberjs/ember-inspector/blob/56575c08bcd3228d830ace596ebb778af2dd4824/ember_debug/libs/render-tree.js#L326

For another use case i would like to have parentElement of in-element to point to the previous parent. Or another way to get it.
Basically with modifiers i am putting them under an html element in inspector. Also all components that are under that element are also put under it. But i cannot know if the in-element was also under that element because it's disconnected

[chancancode]

see all of the above