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

[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

Conversation

fivetanley
Copy link
Member

partially fixes #19869

Due to GlimmerVM's assertion, this still does not fix the issue in strict mode. We also can't work around the strict mode limitation with an AST transform since Glimmer's transforms run first.

partially fixes emberjs#19869

Due to [GlimmerVM's assertion][1], this still does not fix the issue in
strict mode. We also can't work around the strict mode limitation with
an AST transform since Glimmer's transforms run first.

[1]: https://github.com/glimmerjs/glimmer-vm/blob/2ddbbc4a9b97db4f326c4d92021f089c464ab093/packages/%40glimmer/compiler/lib/passes/1-normalization/keywords/utils/curry.ts#L53
@fivetanley fivetanley changed the title [bugfix] allow modifier to resolve built-ins like on [bugfix] allow modifier helper to resolve built-ins like on Jan 24, 2024
@NullVoxPopuli
Copy link
Contributor

Why does ember have its own record of this stuff? I would have thought this behavior (and fix) would be in glimmer-vm, since modifier is a keyword.

@fivetanley
Copy link
Member Author

@NullVoxPopuli I can't speak for the original authors', but the implementation currently rewrites

{{modifier "foo"}}

to

{{modifier (-resolve "modifier:foo")}}

via this AST transform.

The -resolve helper basically does a owner.hasRegistration check and doesn't use the resolver's logic at all.

I'm not sure why Ember has knowledge of these things vs these checks/etc being done in Glimmer itself.

@NullVoxPopuli
Copy link
Contributor

that's goofy.

I wonder if we can instead do all the resolution at build time instead of runtime.

I feel like I have either a very faint memory, or I made one up, of @ef4 mentioning something about this in the last week or two.

Like, automatically converting things to strict-mode so there's no runtime cost to any of this 🤔

@ef4
Copy link
Contributor

ef4 commented Jan 24, 2024

The modifier helper has always been forbidden from accepting strings. It must not accept strings.

The same is true for the helper helper. Only component accepts them, and that only in non-strict mode, because it makes static analysis impossible.

Edit to correct: I see I'm misremember, but I know we have some limitation here. Perhaps it was only accepting string literals.

@ef4
Copy link
Contributor

ef4 commented Jan 24, 2024

But also: do people not realize it's a very bad idea to be using this feature at all in new code in 2024? It moves you further away from adopting template tag and further away from adopting embroider. I promise I will work hard to break it in Ember 6.0.

If you need to yield a contextual modifier, Import your modifier in javascript and use the value directly. That works all the way back to at least Ember 3.28. That's what's aligned with template tag and embroider both.

@fivetanley
Copy link
Member Author

@ef4 should this be the case for built-ins like on ? It feels cumbersome to have to find the import for this (I can't seem to find it in the documentation, had to source dive for it). I noticed @NullVoxPopuli opened several RFCs related to this, one of them making it so on doesn't need an import emberjs/rfcs#997

@fivetanley
Copy link
Member Author

Closing this for now since I found a workaround to get around my problem. This could potentially be revisited in Glimmer itself if the RFCs in my previous comment are accepted. Here's a link to the workaround #19869 (comment)

@fivetanley fivetanley closed this Jan 24, 2024
@ef4
Copy link
Contributor

ef4 commented Jan 25, 2024

Adding more built-in keywords would probably be fine but that's very different from resolving strings to modifiers.

@chancancode
Copy link
Member

That is not correct, this is a long standing bug (linked issue in the original post). All of these accept strings in non-strict mode templates as specified in the RFC; we specifically agreed to make them all work consistently in normal templates and made the static resolution requirement strictly a strict-mode thing.

@chancancode chancancode reopened this Jan 25, 2024
@@ -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.

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.

[Bug] The on modifier does not work with the modifier helper
4 participants