add template only name

[patricklx]

this is for inspector to show template only component names.

templateOnly takes 2 params moduleName and name. https://github.com/glimmerjs/glimmer-vm/blob/10eae7429b702b1e7f5434b91802d5767ff7ad9a/packages/%40glimmer/runtime/lib/component/template-only.ts#L84

and have their defaults set to

  public moduleName = '@glimmer/component/template-only',
  public name = '(unknown template-only component)'

I had to update dependencies to test locally... #59

[patricklx]

I don't think this is the right place to fix this. The correct name to show in the inspector doesn't depend on the definition of the component, it depends on the name used by the caller of the component.

If your template says

import Button from 'fancy-button-addon';

<template>
  <Button />
</template>

The inspector should show <Button />, not whatever name is used inside the component definition in fancy-button-addon.

This would probably be a change in the debugRenderTree area.

I agree, but that information is lost already during precompile. Also what about nested things like this.Button? Or {{component Button}}? Or more complex {{ component (get this this.choose)}} Maybe a label can be added internally? It should actually be done for all parts. Thoughts @chancancode ?

[chancancode]

I don't think this is the right place to fix this. The correct name to show in the inspector doesn't depend on the definition of the component, it depends on the name used by the caller of the component.

If your template says

import Button from 'fancy-button-addon';

<template>
  <Button />
</template>

The inspector should show <Button />, not whatever name is used inside the component definition in fancy-button-addon.

This would probably be a change in the debugRenderTree area.

I'm not sure I agree with that.

The current look and feel of the UI too strongly implies that what shows up in the inspector pane should match your source code, which it is not really designed to do and IMO a non-goal. If we wanted to do that it would require something similar to source-map level of information which we just don't have/would to be expensive to keep around in the current architecture.

I am not saying that we should deliberately make the UI worse, but it's important that we are on the same page about the goals/capabilities/limitations of the current system and not to over extend it/push it in the directions it wasn't meant/designed for. This has also comes up in the past when @patricklx wants to add more runtime/metadata bookkeeping to guarantee the ordering of the tree nodes matches DOM order in all cases, which I personally also think is a non-goal in the current design.

We previously talked about dropping the idea of "module name" entirely, which I think would be fine, as long as we add back a way to find the component. I think @ef4 previous suggested adding a stub function so we can use jump to function definition for the linkage, which I think is a good idea.

Back to the current topic. I would say it's definitely not the intention to have the render node represent/preserve any caller side information, and in any case the actual JS binding name gets lost very quickly and I'm not even sure we are in a position to preserve/propagate it even if we want to.

In terms of the current system, the render node is a hookable API by the component managers, and and it takes the "component definition" as the input. Typically the component definition is just a JS reference to the component class. I suppose you can do something in the JS/babel side to wrap each of these references with an object that preserves the name as a string, but you would be doing that for every instance of every component invocation. IMO that is just not worth it. It will also either have bloat the production payload or make the code very different in dev vs prod, which could cause other problems.

[NullVoxPopuli]

which I personally also think is a non-goal in the current design.

isn't the goal of the debug render tree to make it easy for the tools to debug the render tree -- rather than make up assumptions about how things are invoked? adding more data to the compiled output would increase bundle size.

adding a stub function so we can use jump to function definition for the linkage

what would that look like? :thinking: would these functions actually be evaluated? (if so, what would the performance impact be? (we already have a bad perf regression in the VM :( https://github.com/glimmerjs/glimmer-vm/issues/1590 ))

[chancancode]

The current look and feel of the UI too strongly implies that what shows up in the inspector pane should match your source code, which it is not really designed to do and IMO a non-goal.

I should probably also say what I believe the goal of the inspector pane is. I think it's just supposed to give you best-effort/useful-enough information so that someone who works on the codebase would have a decent shot at correlating the output with the application logic, while limiting the overhead of metadata/bookkeeping we add to make it work. Generally, we try to piggy back on whatever information we already have/need for other purposes (which is why moduleName was used, because it was already there) and avoid adding extra stuff only for the purpose of making this work.

To the extent it is helpful, we can lay things out to match how it looks in the template source code, that's one way to help developers make the correlation, but it's not the primary objective and we don't impose that as a goal on ourselves at the expense of having to capture/carry extra stuff just to make the visualization nice.

I think there is a alternative design where we can make that a goal, but it would need to be a very different architecture where the additional information is completely optional and carried in a side channel like how source maps work.

[patricklx]

I think how the inspector displays the components is good, considering the many ways a component can be obtained through functions etc it should just show the component name. in addition <this.MyCurrenComponent /> could be a getter which returns different components depending on state. showing the component name in use is more useful then.

what could be done in addition is to add a label to it (if at some point we get it from glimmer-vm) and have options in the inspector to inline it in the component tree or have them as tooltip.

it will probably take some time until that lands in glimmer. if at all. until then, i think it makes sense to merge this PR?

[ef4]

and we don't impose that as a goal on ourselves at the expense of having to capture/carry extra stuff just to make the visualization nice.

This PR is literally capturing extra information just to make the visualization marginally nicer, while also sometimes making it more misleading. I don't think it's worth it.

I do think we will ultimately want to capture extra debug information, but in a separate bundle that doesn't effect normal app usage.

[ef4]

Another problem here: this feature only works in the pre-rfc-931 API. Right now, the babel plugin only uses that API. But we don't want to be stuck with that. Apps could all already use the post-rfc-931 API and get smaller code (we just haven't gotten around to making that improvement here), and that API intentionally does not accept a component name argument.

[patricklx]

Another problem here: this feature only works in the pre-rfc-931 API. Right now, the babel plugin only uses that API. But we don't want to be stuck with that. Apps could all already use the post-rfc-931 API and get smaller code (we just haven't gotten around to making that improvement here), and that API intentionally does not accept a component name argument.

So we will end up needing an alternative anyway? would that also require changes to glimmer-vm or only in the babel plugin? I think another way how it could be done is by providing an inline name for a template only component. e.g <template name='my-component'></template>

[chancancode]

and we don't impose that as a goal on ourselves at the expense of having to capture/carry extra stuff just to make the visualization nice.

This PR is literally capturing extra information just to make the visualization marginally nicer...

I said a lot of things and it's pretty nuanced, I didn't mean for it to boil down to "let's declare a moratorium on new metadata (though my personal position is not far off from that).

I am not here to endorse this particular PR. I do think there is some value in keeping the status quo of the inspector working enough as we make these other changes, so to the extent it's not too onerous and the amount of metadata is inline with what the previous status quo, that feels okay to me personal, but:

I don't think it's worth it.

Yes I think you should make those calls. I also pointed out an alternative (what IIRC you originally proposed elsewhere) is to attach a stub function to the components, either to capture a stack trace or to use the jump to function definition feature to link the component, which I think is a pretty neat idea. (Though we probably still need some kind of text label for any form of visualization.

What I was largely responding to was the original reason for rejecting the PR was:

The inspector should show <Button />, not whatever name is used inside the component definition in fancy-button-addon.

This would probably be a change in the debugRenderTree area.

This would require pushing the metadata capturing to the invocation side, which would massively increase the amount of information we need to capture and also not a good fit for the current debugRenderTree design.

...while also sometimes making it more misleading.

If you are talking about multiple components in the same file, or that the file name/path is not a particularly good indicator anyway, then 100%. I would totally be onboard with tweaking the debug label API and see about capturing the identifier we assigned it to, etc.

What I disagree with is that the name should/has to match the source text form the invocation side to be "accurate", and that it is a design goal of the current inspector/debugRenderTree API. And that has been a mild disagreement brewing/something I have been trying to explain to @patricklx and @NullVoxPopuli for a while.

I understand that the current UI is a bit uncanny in that it resembles your source code a bit too much and so when they don't match up exactly it seems "surprising" and feels like something needs fixing. But really it's just an abstracted/lossy reconstructed view of the runtime information we have, and it's by design (or at least, the lack of a better design).

Keep pushing on adding more metadata to the current system such in the direction of reconstructing a more accurate view of the template source is like adding metadata to an AST so that it can eventually re-print the source code. Sometimes you can get away with it, but ultimately it's a pretty poor fit. And more importantly in our case, all those metadata we capture has runtime costs today.

I do think we will ultimately want to capture extra debug information, but in a separate bundle that doesn't effect normal app usage.

That we are on the same page and is what I meant by:

I think there is a alternative design where we can make that a goal, but it would need to be a very different architecture where the additional information is completely optional and carried in a side channel like how source maps work.

It is a fair bit of work and needs careful design though. IMO as long as we accept that the current system is meant to be a lossy/best-effort view that gives you enough information to point you at the right direction, there is still plenty of millage left there without blowing everything up. It's really not worse than things like function names in stack traces and flame graphs where the information is there just a bit lossy.

[ef4]

Yeah I think we are pretty aligned. I see why my original suggestion is a bridge too far for the current design and would require a wholly different system.

[patricklx]

So, anything that can be done near feature to not have many unknown components in the inspector tree? Maybe this with some opt-in/out flags for dev & release?

[patricklx]

I found that this was the default behaviour in ember template imports v3 https://github.com/ember-template-imports/ember-template-imports/blob/v3.4.2/src/babel-plugin.js#L42

[patricklx]

I think this makes more sense to add back to ember-template-imports. because this is only needed if ember-template-imports is used