delegate modifiers debug name & instance

[patricklx]

currently arrow functions or functions without name are displayed with their whole implementation source code

closes #1589

[NullVoxPopuli]

what will this look like?

[patricklx]

what will this look like?

{{my-modifier fn}} is how it can look like.

Currently... look at you limber site :) it's a good example

[NullVoxPopuli]

I see!

looks like anything that fixes this is good for me

[patricklx]

@NullVoxPopuli actually, current release of inspector cannot handle latest ember... need nightly:

[NullVoxPopuli]

Why definition.state here, if the argument is 'definition' (for modifiers)?

Is this an inconsistency between modifiers and components managers?

[patricklx]

From what i understand: Because state is from the 'main' manager, and definition.state is from the delegate. And state has the delegate needed to delegate to function call

[wycats]

This looks good to me. Let's do it!

[chancancode]

This seems fishy, what is that second arg?

[patricklx]

renamed the type here to be the same as in modifier.d.ts file

[chancancode]

I don't think we'd need to store it since it gets passed in?

[NullVoxPopuli]

Seems good

[NullVoxPopuli]

is this still needed?

[NullVoxPopuli]

tiny cleanup thing, otherwise 👍

[patricklx]

how about making getDebugName static? @chancancode

[chancancode]

I see, the various names we use ("delegate" vs "manager" vs "internal manager" vs ...) is a bit confusing but I think what is going on is:

1. Fundamentally, as far as Glimmer is concerned:

   • Modifier managers (~= factories..?) are static are are used to decide how to instantiate things and make other policy decisions
   • Modifier definitions – a class, a function, etc, but can be whatever the manager knows how to instantiate; you can make a modifier manager that knows to to use the POJO { name: "my-modifier" } as a modifier
   • Modifier instance – in the case of a class, this would possibly be the instance, but it's generalized into whatever the "instantiated thing" is and the manager may use that to store its own state/metadata as well, ultimately we treat this as an "opaque state bucket"

2. As discussed in previous iterations of this PR, we want the ability to name a modifier "statically" – in this context meaning just the "definition" (class, function, etc), as opposed to the instance. This allows this naming feature to be used in more cases, such as the error message when we failed to construct an instance

3. So far so good. However, Ember introduced a wrinkle to this – it extended the managers interface to allow dependency injection, so managers are no longer truly static, but rather static as in "one per owner (app/engine/etc)". Which is what you are noticing here.

I suppose the short term fix is to pass the owner here. Other than making the internal API looking a bit random, there isn't really a real problem – realistically when we want to generate an error message based on just the definition (error during construction), we almost certainly are going to have an owner as well. The API awkwardness is also contained in this internal version of the API, and people implementing the custom managers API won't really need to see that.

Perhaps in the long term it can be revisited if Ember really needs DI on the managers, and can we just make them truly static.

[patricklx]

yeah, I tried that, the problem is in

glimmer-vm/packages/@glimmer/runtime/lib/environment.ts

Line 68 in 10eae74

definition.resolvedName || manager.getDebugName(definition.state)

there is no direct way to access the owner, nor vm.owner(), maybe more changes would be requried?

[chancancode]

oof, sorry this has been rough. It's the code equivalent of random bureaucracy. Notionally we have the information, just not consistently threaded/easily accessible.

Since we narrowed this problem down to these private APIs (I think, this class is private, right?), I don't really care that much about the API surface.

I think the API surface of what we call the "delegate" here is important (that's what people actually implement right?), and I think we got that down pretty good.

As long as we don't affect the public API, we can do anything (that is type safe, etc) for this wrapper class. Maybe we keep getDebugName(owner, definition) and getDebugNameFromInstance(instance) in this internal class and call it a day for now. For reference: this is the kind of place we may want to use the former, which we do have the owner:

glimmer-vm/packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts

Lines 147 to 153 in 34888e9

	let capturedArgs = args.capture();
	let state = manager.create(
	owner,
	expect(constructing, 'BUG: ElementModifier could not find the element it applies to'),
	definition.state,
	capturedArgs
	);

For what its worth I think a lot of this up is overdue for a cleanup/rethink pass, especially for the manages stuff. The pure form of the idea is good, but we made some decisions along the way that incidentally complicated things a fair bit.

Dismissing because we want to go more in a direction that exposes what the user typed, rather than try to derive debug aliases / names from existing runtime information --- here is an example: #1634 -- @wycats is currently thinking about this PR and is actively workingish on it -- this way when folks import something and alias it, we use the alias name, rather than the name it was defined with (current behavior as implemented)