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.

Sign up for GitHub

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

Jump to bottom

delegate modifiers debug name & instance #1591

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+27 −23
Open

delegate modifiers debug name & instance #1591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
544d222
fix arrow modifiers debug name
patricklx Jun 25, 2024
56d1454
allow if custom toString
patricklx Jun 25, 2024
c66cae8
delegate call to delegate
patricklx Jun 26, 2024
3c7f0de
pass owner
patricklx Jun 26, 2024
b32c947
use name
patricklx Jun 26, 2024
2d67837
fix types
patricklx Jun 26, 2024
8105426
fix test
patricklx Jun 26, 2024
2262c85
fix lint
patricklx Jun 26, 2024
566e047
revert
patricklx Jun 26, 2024
25d103d
return delegate if no modifier
patricklx Jun 26, 2024
01399dd
cleanup code
patricklx Jul 11, 2024
d02dedf
fix
patricklx Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@glimmer-workspace/integration-tests/lib/test-helpers/define.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class FunctionalModifierManager implements ModifierManager<SimpleModifierState>
}
}

getDebugName(fn: Function) {
getDebugName(fn: SimpleModifierFn) {
return fn.name || '(anonymous function)';
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/@glimmer/interfaces/lib/managers/modifier.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,6 @@ export interface ModifierManager<ModifierStateBucket> {
installModifier(instance: ModifierStateBucket, element: Element, args: Arguments): void;
updateModifier(instance: ModifierStateBucket, args: Arguments): void;
destroyModifier(instance: ModifierStateBucket, args: Arguments): void;
getDebugName?(factory?: unknown): string;
getDebugInstance?(instance: ModifierStateBucket): any;
}
46 changes: 24 additions & 22 deletions packages/@glimmer/manager/lib/public/modifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,12 @@ export function modifierCapabilities<Version extends keyof ModifierCapabilitiesV
});
}

export interface CustomModifierState<ModifierInstance> {
export interface CustomModifierState<ModifierStateBucket> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

tag: UpdatableTag;
element: SimpleElement;
modifier: ModifierInstance;
delegate: ModifierManager<ModifierInstance>;
modifier: ModifierStateBucket;
delegate: ModifierManager<ModifierStateBucket>;
args: Arguments;
debugName?: string;
}

/**
Expand All @@ -65,12 +64,12 @@ export interface CustomModifierState<ModifierInstance> {
* `updateModifier()` - invoked when the arguments passed to a modifier change
* `destroyModifier()` - invoked when the modifier is about to be destroyed
*/
export class CustomModifierManager<O extends Owner, ModifierInstance>
implements InternalModifierManager<CustomModifierState<ModifierInstance>>
export class CustomModifierManager<O extends Owner, ModifierStateBucket>
implements InternalModifierManager<CustomModifierState<ModifierStateBucket>>
{
private componentManagerDelegates = new WeakMap<O, ModifierManager<ModifierInstance>>();
private componentManagerDelegates = new WeakMap<O, ModifierManager<ModifierStateBucket>>();

constructor(private factory: ManagerFactory<O, ModifierManager<ModifierInstance>>) {}
constructor(private factory: ManagerFactory<O, ModifierManager<ModifierStateBucket>>) {}

private getDelegateFor(owner: O) {
let { componentManagerDelegates } = this;
Expand Down Expand Up @@ -99,41 +98,44 @@ export class CustomModifierManager<O extends Owner, ModifierInstance>
let delegate = this.getDelegateFor(owner);

let args = argsProxyFor(capturedArgs, 'modifier');
let instance: ModifierInstance = delegate.createModifier(definition, args);
let modifier: ModifierStateBucket = delegate.createModifier(definition, args);

let tag = createUpdatableTag();
let state: CustomModifierState<ModifierInstance>;
let state: CustomModifierState<ModifierStateBucket>;

state = {
tag,
element,
delegate,
args,
modifier: instance,
modifier,
};

registerDestructor(state, () => delegate.destroyModifier(instance, args));
registerDestructor(state, () => delegate.destroyModifier(modifier, args));

return state;
}

getDebugName(definition: object) {
if (typeof definition === 'function') {
return definition.name || definition.toString();
} else {
return '<unknown>';
const delegate = this.factory.prototype;
if (typeof delegate?.getDebugName === 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about making getDebugName static? @chancancode

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@patricklx patricklx Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return delegate.getDebugName(definition);
}
return (definition as any).name || '<unknown>';
}

getDebugInstance({ modifier }: CustomModifierState<ModifierInstance>) {
return modifier;
getDebugInstance({ delegate, modifier }: CustomModifierState<ModifierStateBucket>) {
if (typeof delegate?.getDebugInstance === 'function') {
return delegate.getDebugInstance(modifier);
}
return modifier || delegate;
}

getTag({ tag }: CustomModifierState<ModifierInstance>) {
getTag({ tag }: CustomModifierState<ModifierStateBucket>) {
return tag;
}

install({ element, args, modifier, delegate }: CustomModifierState<ModifierInstance>) {
install({ element, args, modifier, delegate }: CustomModifierState<ModifierStateBucket>) {
let { capabilities } = delegate;

if (capabilities.disableAutoTracking === true) {
Expand All @@ -143,7 +145,7 @@ export class CustomModifierManager<O extends Owner, ModifierInstance>
}
}

update({ args, modifier, delegate }: CustomModifierState<ModifierInstance>) {
update({ args, modifier, delegate }: CustomModifierState<ModifierStateBucket>) {
let { capabilities } = delegate;

if (capabilities.disableAutoTracking === true) {
Expand All @@ -153,7 +155,7 @@ export class CustomModifierManager<O extends Owner, ModifierInstance>
}
}

getDestroyable(state: CustomModifierState<ModifierInstance>) {
getDestroyable(state: CustomModifierState<ModifierStateBucket>) {
return state;
}
}
Expand Down
Loading