Skip to content
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 template only name #58

Closed
wants to merge 7 commits into from
Closed

Conversation

patricklx
Copy link
Contributor

@patricklx patricklx commented Jun 25, 2024

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 patricklx force-pushed the template-only-name branch from bd1f293 to 1fade50 Compare June 25, 2024 10:00
src/expression-parser.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -1468,7 +1468,7 @@ describe('htmlbars-inline-precompile', function () {
import { precompileTemplate } from "@ember/template-compilation";
import { setComponentTemplate } from "@ember/component";
import templateOnly from "@ember/component/template-only";
export default setComponentTemplate(precompileTemplate('<HelloWorld @color={{"#ff0000"}} />', { scope: () => ({ HelloWorld }), strictMode: true }), templateOnly());
export default setComponentTemplate(precompileTemplate('<HelloWorld @color={{"#ff0000"}} />', { scope: () => ({ HelloWorld }), strictMode: true }), templateOnly(undefined, "foo-bar"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be PascalCase, since it's what would get shown in the inspector, and how it would be invoked by a consumer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspector would auto transform it to pascal case

Copy link
Contributor Author

@patricklx patricklx Jun 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

inspector:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it also doesn't matter if it starts with lower case. so my-template-only-2 would also be shown as MyTemplateOnly2

@patricklx patricklx force-pushed the template-only-name branch from 2761187 to c99903e Compare June 25, 2024 11:42
NullVoxPopuli
NullVoxPopuli previously approved these changes Jun 25, 2024
@NullVoxPopuli NullVoxPopuli dismissed their stale review June 25, 2024 15:33

perhaps we should handle this in the glimmer-vm's debug render tree? since the templats / VM have to know the names at the call sites, which is more accurate for showing in the inspector?

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@patricklx
Copy link
Contributor Author

patricklx commented Jun 25, 2024

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
Copy link
Member

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
Copy link
Contributor

NullVoxPopuli commented Jun 25, 2024

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? 🤔
would these functions actually be evaluated? (if so, what would the performance impact be? (we already have a bad perf regression in the VM :( glimmerjs/glimmer-vm#1590 ))

@chancancode
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

ef4 commented Jun 27, 2024

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.

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to come up with descriptions here we should try to make them actually distinct for distinct components. These are not.

src/plugin.ts Outdated
@@ -496,14 +497,27 @@ function insertCompiledTemplate<EnvSpecificOptions>(

let expression = t.callExpression(templateFactoryIdentifier, [templateExpression]);

let assignment = target.parent;
let assignmentName: t.StringLiteral = t.stringLiteral(state.filename);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can leak whole filesystem paths, which is both bloat and information leak. Also, it's not even a good answer, because you can have many components in this same file and they will all look as if they're the same component in the inspector, leading to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, should be the same as below, babel.types.identifier('undefined')

src/plugin.ts Outdated
let assignment = target.parent;
let assignmentName: t.StringLiteral = t.stringLiteral(state.filename);
if (assignment.type === 'AssignmentExpression' && assignment.left.type === 'Identifier') {
assignmentName = t.stringLiteral(assignment.left.name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using local variables like this is also likely to mislead, because nothing is stopping people from reusing the same local variable names in different modules that interact with each other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its the best we can do right now. at least better than unknown template.
how about basename of file + variable name?

@ef4
Copy link
Contributor

ef4 commented Jun 27, 2024

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
Copy link
Contributor Author

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
Copy link
Member

chancancode commented Jun 27, 2024

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
Copy link
Contributor

ef4 commented Jun 27, 2024

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
Copy link
Contributor Author

patricklx commented Jun 27, 2024

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
Copy link
Contributor Author

patricklx commented Jun 28, 2024

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
Copy link
Contributor Author

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

@patricklx patricklx closed this Jul 8, 2024
@patricklx patricklx deleted the template-only-name branch July 8, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants