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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] The on modifier does not work with the modifier helper #19869

Open
lolmaus opened this issue Dec 2, 2021 · 7 comments 路 May be fixed by #20629
Open

[Bug] The on modifier does not work with the modifier helper #19869

lolmaus opened this issue Dec 2, 2021 · 7 comments 路 May be fixed by #20629
Labels

Comments

@lolmaus
Copy link
Contributor

lolmaus commented Dec 2, 2021

馃悶 Describe the Bug

This works:

<div {{(if this.condition (modifier 'my-modifier' this.bar))}}>

馃敩 Minimal Reproduction

This doesn't:

<div {{(if this.condition (modifier 'on' 'scroll' this.bar))}}>

Can't create a Twiddle because Twiddle is severely outdated, sorry.

馃槙 Actual Behavior

Uncaught Error: Assertion Failed: Attempted to invoke (-resolve "modifier:on"), but on was not a valid modifier name.

馃 Expected Behavior

Should apply the on modifier to the element.

馃實 Environment

  • Ember: 3.28.4
  • Node.js/npm: 14.16.0
  • OS: Build: Ubuntu, Browser: Windows 10
  • Browser: Chrome, Firefox

CC @simonihmig

@Windvis
Copy link
Contributor

Windvis commented Feb 3, 2022

I did some testing and it's not only the on modifier that has this problem. It seems that all helpers and modifiers that come from Glimmer don't work with the contextual helpers.

The error is thrown from the -resolve helper which gets wrapped around the string by a template transform plugin.

The built-in modifiers and helpers don't seem to be registered in the container which is why the -resolve helper throws that error. It is possible to work around this in app code by importing the helper or modifier and manually registering it.

For example:

import { getOwner } from '@ember/application';
import { on } from '@ember/modifier';

export default class ApplicationRoute extends Route {
  constructor() {
    super(...arguments);
    getOwner(this).register('modifier:on', on);
  }
}

ember-source contains the setupEngineRegistry util that already registers some built in components, so adding all the needed registrations there would be a potential fix for this problem. The question is, is that the correct fix or are there better alternatives?

@Windvis
Copy link
Contributor

Windvis commented Feb 8, 2022

I found the following in the RFC:

Some built-in helpers or modifiers may not be resolvable with the helper and modifier helpers. For example, (helper "debugger") and (helper "yield") will not work, as they are considered keywords. For implementation simplicity, we propose to forbid resolving built-in helpers, components and modifiers this way across the board (i.e. a runtime error). We acknowledge that there are good use cases for this feature such as currying the array and hash helpers, and will consider enabling them in the future on a case-by-case basis.

So it seems like these are not supported by design and it isn't actually a bug? In that case a more clear error message would still be nice though.

Does that mean we need an RFC for this?

@chancancode
Copy link
Member

@Windvis I think that sections are referring to cases that are really "keywords" that aren't really helpers, debugger and yield are good examples, pretty sure (debugger) and (yield). {{mount}} and {{outlet}} also comes to mind.

But I am pretty sure (modifier "on") is supposed to work, the RFC had a bunch of examples showing just that. So I think this is an implementation issue (i.e. a bug). I'll confirm tomorrow.

@chancancode
Copy link
Member

chancancode commented Apr 1, 2022

Presumably on isn't the only case? Are there other built-in things that doesn't work?

@chancancode
Copy link
Member

Yes, this should work. In addition to fixing this bug we should also do a pass on all the other helpers/modifiers/keywords/RFCs we have to see if there is anything else missing, but they don't have to block each other.

@yads
Copy link

yads commented Mar 3, 2023

A use case for this is to conditionally add a click handler to an element e.g.

<div role="{{if this.doButton 'button'}}" {{(if this.doButton (modifier "on" "click" this.handleClick))}}>
  Click Me
</div>

Currently my workaround is to have a modifier that adds an event listener conditionally

export default modifier(function conditionalOn(
  element,
  [condition, event, handler],
  { capture, once, passive }
) {
  if (condition) {
    element.addEventListener(event, handler, { capture, once, passive });
  }
});

fivetanley added a commit to fivetanley/ember.js that referenced this issue Jan 24, 2024
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 added a commit to fivetanley/ember.js that referenced this issue Jan 24, 2024
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
Copy link
Member

For future travelers: As a workaround, here's what I ended up doing (thank you @NullVoxPopuli for helping me find the import):

// app/components/my-component.js

import Component from '@glimmer/component';
import { on } from '@ember/modifier';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';

export default class MyComponent extends Component {
  on = on;
  @tracked bindClickHandler = true;
  @action handleClick() {
    this.
  }
}

then my template looks like

<button
  {{(if this.bindClickHandler (modifier this.on "click" this.handleClick))}}
>Click me
</button>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants