[FEATURE] Implement on modifier.
#17960
Conversation
| import { isChrome, isFirefox } from '@ember/-internals/browser-environment'; | ||
| import { privatize as P } from '@ember/-internals/container'; | ||
|
|
||
| const isIE11 = !window.ActiveXObject && 'ActiveXObject' in window; |
| callback: EventListener, | ||
| options?: AddEventListenerOptions | ||
| ): void { | ||
| adds++; |
There was a problem hiding this comment.
Assuming these aren't in DEBUGs because you wanted to run these tests against IE?
There was a problem hiding this comment.
Yes, exactly. We can ultimately remove if we care, but it's very very cheap and almost certainly won't matter.
rwjblue
force-pushed
the
add-on-modifier
branch
from
27f9c3e
to
2c6a039
Compare
| updateFromArgs() { | ||
| let { args } = this; | ||
|
|
||
| let { once, passive, capture }: AddEventListenerOptions = args.named.value(); |
There was a problem hiding this comment.
In ember-on-modifier, we assert that only these three options are passed to guard against typos. Do you think we should add this here as well?
There was a problem hiding this comment.
Hmm, the RFC specifically states the opposite (that passing other things is "ignored"):
From here:
The following named parameters will be accepted as options to {{on}}:
- capture
- once
- passive
These will be passed forward to addEventListener as options in modern browsers, and will be polyfilled in older browsers. Other options will be ignored.
I don't feel super strongly so I just followed the RFC on this.
@pzuraq - Do you think we should change to assert?
| addEventListener(state.element, state.eventName, state.callback, state.options); | ||
|
|
||
| state.shouldUpdate = false; | ||
| } |
There was a problem hiding this comment.
If I'm reading this correctly, once the modifier is installed, it will only ever update to a new listener, but will never be uninstalled. I can't think of a scenario, where the modifier would need to be uninstalled from an element that does not get removed from the DOM afterwards, but I think @chancancode made a case for this once on Discord.
There was a problem hiding this comment.
There was a problem hiding this comment.
@buschtoens - We definitely cleanup (see getDestructor just below, and the destroy method on the OnModifierState object). I will also add a test (using the assert count methodology) to ensure we don't accidentally regress this.
There was a problem hiding this comment.
Ah, I see! So getDestructor returns a Destroyable, which is implemented through destroy() in OnModifierState. I was not familiar with that part of the Glimmer API yet. 👍
There was a problem hiding this comment.
Ya, this is one way the "internal" modifiers differ from the public API. The public API effectively does the same thing (the destroy method on the state bucket invokes the destroyModifier hook on the custom manager).
packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/modifiers/on-test.js
Show resolved
Hide resolved
See https://emberjs.github.io/rfcs/0471-on-modifier.html for details. Special thanks to [buschtoens](https://github.com/buschtoens)'s work on [ember-on-modifier](https://github.com/buschtoens/ember-on-modifier) which heavily inspired this implementation.
rwjblue
force-pushed
the
add-on-modifier
branch
from
2c6a039
to
9e0213f
Compare
* Extract shared utility for a `this` context that errors when used. * Assert when more positional arguments are provided. * Avoid wrapping callback needlessly (makes debugging a bit easier, since the event handler function is actually the user-land function in most cases. * Incorporates a system to error if any `this` properties are accessed from within the callback in debug builds. * Fixup comments for addEventListener / removeEventListener logic. * Fix test assertion message. * Add test ensuring the modifier is destroyed properly.
rwjblue
force-pushed
the
add-on-modifier
branch
from
9e0213f
to
7e0cd03
Compare
|
FYI - Updated to address review feedback:
I think this should be good to go once CI is green, in case anyone wants another crack at a review first... |
| ); | ||
| } | ||
|
|
||
| const context = buildUntouchableThis('`fn` helper'); |
| }); | ||
| } else if (DEBUG) { | ||
| // prevent the callback from being bound to the element | ||
| this.callback = userProvidedCallback.bind(untouchableContext); |
|
@rwjblue The current implementation of |
See https://emberjs.github.io/rfcs/0471-on-modifier.html for details.
Special thanks to @buschtoens's work on ember-on-modifier which heavily inspired this implementation.