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

[bugfix] allow modifier helper to resolve built-ins like on #20629

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −1
Open

[bugfix] allow modifier helper to resolve built-ins like on #20629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/@ember/-internals/glimmer/lib/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ const BUILTIN_KEYWORD_MODIFIERS: Record<string, ModifierDefinitionState> = {
action: actionModifier,
};

const BUILTIN_MODIFIERS: Record<string, object> = {
export const BUILTIN_MODIFIERS: Record<string, object> = {
...BUILTIN_KEYWORD_MODIFIERS,
on,
};
Copy link
Member

Choose a reason for hiding this comment

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

This is used in lookupModifier on L219. The problem is, it seems like, (modifier "...") didn't get implemented in glimmer-vm, or at least when we landed this feature in ember, it didn't, so ember added an AST transform hack to change (modifier "...") into (modifier (-resolve "modifier:...")). If that was implemented in glimmer-vm properly like (component "...") is, then it should/would have gone through the codepath in L219 and everything should work. That would be the most proper fix.

I was a bit hesitant on fixing it by adding them to the resolver, but I suppose that would be the most consistent. So I suppose why not.

However, I do think we need to fix this properly upstream; the problem with the current AST-transform hack in Ember is it will, as far as I can tell, still do the same AST transform in strict mode templates, which is probably punching a pretty big hole into the entire Embroider/static template universe.

Expand Down
5 changes: 5 additions & 0 deletions packages/@ember/-internals/glimmer/lib/setup-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { Renderer } from './renderer';
import OutletTemplate from './templates/outlet';
import RootTemplate from './templates/root';
import OutletView from './views/outlet';
import { BUILTIN_MODIFIERS } from './resolver';

export function setupApplicationRegistry(registry: Registry): void {
// because we are using injections we can't use instantiate false
Expand Down Expand Up @@ -57,4 +58,8 @@ export function setupEngineRegistry(registry: Registry): void {
if (!ENV._TEMPLATE_ONLY_GLIMMER_COMPONENTS) {
registry.register(P`component:-default`, Component);
}

for (let [name, modifier] of Object.entries(BUILTIN_MODIFIERS)) {
registry.register(`modifier:${name}`, modifier, { instantiate: false });
}
}
22 changes: 22 additions & 0 deletions packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,28 @@ moduleFor(
this.assertStableRerender();
}

'@test can resolve built-in modifiers'(assert) {
let wasCalled = false;
let id = 'wow-what-an-original-id';
this.render(
`<div id='${id}' {{(if this.isModifying (modifier "on" "click" this.callAction))}} />`,
{
callAction: () => {
wasCalled = true;
},
}
);

let element = document.querySelector(`#${id}`);
element.click();

assert.false(wasCalled, 'modifier should not be set up');

runTask(() => set(this.context, 'isModifying', true));
element.click();
assert.true(wasCalled, 'on modifier can be used');
}

'@test Cannot dynamically resolve a modifier'(assert) {
this.registerModifier(
'replace',
Expand Down
Loading