-
Notifications
You must be signed in to change notification settings - Fork 424
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
Introduce once and passive events to actions #232
Conversation
Nice work! I was wondering if it would be possible to expand the API a bit to ensure that more than one event listener option can be added per action and provide support for other options such as I'm not sure if it should be a whitelisted set of properties or if any arbitrary property should be accepted. Also, allowing custom values per option makes sense for properties like I considered using a quoteless JSON format, but it doesn't feel very easy on the eyes: <div data="event{bubbles:false,once:true}->controller#action"> Perhaps it could be along the lines of: <div data="event{bubbles:false|once:true}->controller#action"> |
Thanks
I am not sure to really understand, maybe I am missing somehting here but For |
You're totally right, I'm sorry about the confusion! I got the options of the event listener and the events themselves mixed up 😕 Thanks for taking the time to sort it out. |
There are still good reasons for providing a key/value syntax to support all event listener options. For example, |
ok thanks, both for your inputs Does it makes sens to have |
I'm tempted to suggest antonyms for the options like passive / active and once / always, but it might be too ambiguous since passive / once represents actual option names and active / always does not. I know Sam mentioned that he liked the dot notation, but I'm wondering if it's too close to the target descriptor? The JSON notation |
Here’s my syntax suggestion: <!--passive: true-->
<div data-action="touchmove[passive]->controller#method"></div>
<div data-action="touchmove[passive]@window->controller#method"></div>
<!--once: true-->
<button data-action="click[once]->controller#vote"></button>
<div data-action="keydown[once]@window->controller#delete"></div>
<button data-action="[once]controller#vote"></button>
<!--passive: false-->
<div data-action="touchmove[!passive]->controller#method"></div>
<!--passive: false, once: true-->
<div data-action="touchmove[!passive][once]->controller#method"></div>
|
Thinking through my proposal a bit more, there’d really be only four new tokens to match: |
How about: |
In this case, I think parens are a bad idea because they look like arguments. A common question is how to pass arguments to action methods. I can see how someone might confuse a parenthesized option syntax with arguments being passed to the action itself. |
After some additional thought on this, I think within the various current proposals we have too much syntax variants for this feature:
It makes it hard to remember where and how to put the options Suggestion use the same syntax as the addEventListenerAt the end what we are doing is specifying all of the arguments of the event listener. My idea is to keep the order and syntax as close as possible to the Keeping the same order would translate to such a syntax Moving the options to the end simplifies the syntaxes, options always at the end whether there is a global event or a default event and we keep the same order so it is easy to remember. Bracket syntax<!--passive: true-->
<div data-action="touchmove->controller#method[passive]"></div>
<div data-action="touchmove@window->controller#method[passive]"></div>
<!--once: true-->
<button data-action="click->controller#vote[once]"></button>
<button data-action="controller#vote[once]"></button>
<!--passive: false-->
<div data-action="touchmove->controller#method[!passive]"></div>
<!--passive: false, once: true-->
<div data-action="touchmove->controller#method[!passive][once]"></div> Dot syntax<!--passive: true-->
<div data-action="touchmove->controller#method.passive"></div>
<div data-action="touchmove@window->controller#method.passive"></div>
<!--once: true-->
<button data-action="click->controller#vote.once"></button>
<button data-action="controller#vote.once"></button>
<!--passive: false-->
<div data-action="touchmove->controller#method!passive"></div>
<!--passive: false, once: true-->
<div data-action="touchmove->controller#method!passive.once"></div>
<div data-action="touchmove->controller#method.once!passive"></div> |
I like the idea of moving the suffix to the action and reduce the number of variations. That might have been why I initially mixed up the event and listener options: they were declared at the event part of the descriptor instead of at the action (which plays the role of a listener). |
Fair point about matching the argument order of If we put options on the right, I don’t think we should use brackets or dots. They both look too much like property accessors. A colon works nicely (and evokes CSS pseudo-selector syntax): <!--passive: true-->
<div data-action="touchmove->controller#method:passive"></div>
<div data-action="touchmove@window->controller#method:passive"></div>
<!--once: true-->
<button data-action="click->controller#vote:once"></button>
<button data-action="controller#vote:once"></button>
<!--passive: false-->
<div data-action="touchmove->controller#method:passive"></div>
<!--passive: false, once: true-->
<div data-action="touchmove->controller#method:!passive:once"></div> |
ok thanks, @sstephenson looks like we all together came up to a nice plan. Will update the code accordingly |
Just wanted to pop in and say that it's neat to see how y'all worked through this together. 👏 Great job, and thanks for doing this kind of work out in the open so others like me can benefit! |
4b62ee2
to
f01a72b
Compare
I have updated the code to reflect this new syntax. allowed tokens are Several comments:
A typical example would be a button to vote, first click increase a counter with stopPropagation and subsequent clicks display an alert message <button data-action="controller#vote:once controller#alert"></button> To do so I modified the key of the
|
We can get all tests green by using the Github Polyfill AND explicitly passing the handleEvent function to the |
I'd rather not make code changes to work around an incomplete polyfill. Perhaps we can instead update the polyfill to support the |
Ok @javan I ll try to open a PR on the respective repo with the support for |
I opened github/eventlistener-polyfill#13 to add |
Thanks @javan I was caught up on some other projects and didn't have time yet to look into it. That is great. Do you want that I update this branch with the polyfill code directly into We can then decide whether we provide our own polyfill or wait for the PR to be merged and published |
No need to add the code directly. You can apply this change to test with my fork: View diffdiff --git a/packages/@stimulus/core/src/event_listener.ts b/packages/@stimulus/core/src/event_listener.ts
index 018e2bf..3254fe2 100644
--- a/packages/@stimulus/core/src/event_listener.ts
+++ b/packages/@stimulus/core/src/event_listener.ts
@@ -14,11 +14,11 @@ export class EventListener implements EventListenerObject {
}
connect() {
- this.eventTarget.addEventListener(this.eventName, this.handleEvent, this.eventOptions)
+ this.eventTarget.addEventListener(this.eventName, this, this.eventOptions)
}
disconnect() {
- this.eventTarget.removeEventListener(this.eventName, this.handleEvent, false)
+ this.eventTarget.removeEventListener(this.eventName, this, false)
}
// Binding observer delegate
@@ -33,7 +33,7 @@ export class EventListener implements EventListenerObject {
this.unorderedBindings.delete(binding)
}
- handleEvent = (event: Event) => {
+ handleEvent(event: Event) {
const extendedEvent = extendEvent(event)
for (const binding of this.bindings) {
if (extendedEvent.immediatePropagationStopped) {
diff --git a/packages/@stimulus/polyfills/index.js b/packages/@stimulus/polyfills/index.js
index 0280022..e3b67d6 100644
--- a/packages/@stimulus/polyfills/index.js
+++ b/packages/@stimulus/polyfills/index.js
@@ -7,4 +7,4 @@ import "core-js/fn/promise"
import "core-js/fn/set"
import "element-closest"
import "mutation-observer-inner-html-shim"
-import "eventlistener-polyfill"
+import "eventlistener-polyfill/src"
diff --git a/packages/@stimulus/polyfills/package.json b/packages/@stimulus/polyfills/package.json
index b6459f5..0d6e819 100644
--- a/packages/@stimulus/polyfills/package.json
+++ b/packages/@stimulus/polyfills/package.json
@@ -13,7 +13,7 @@
"core-js": "^2.5.3",
"element-closest": "^2.0.2",
"mutation-observer-inner-html-shim": "^1.0.0",
- "eventlistener-polyfill": "^1.0.4"
+ "eventlistener-polyfill": "javan/eventlistener-polyfill#event-listener-interface"
},
"publishConfig": {
"access": "public" |
All green 🚀 |
Thanks @javan for your contribution to this polyfill. I have updated the Stimulus/Polyfill package to point to this latest release and it works perfectly 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely work on this, @adrienpoly.
I can think of one other case that might be problematic.
Every action contains an index, a number representing the position of the descriptor token inside the data-action
value.
Whenever the value of a data-action
attribute changes, Stimulus destroys the bindings previously associated with the action and then binds the new actions. Under the hood, that means event listeners are removed and re-installed.
What will happen in this scenario?
- Load an element with a
once
action:<button data-action="click->controller#a:once">
- Click the button; the
a
method is called. - Click the button again; nothing happens, as expected.
- Modify the button’s
data-action
attribute withsetAttribute()
:<button data-action="click->controller#b click->controller#a:once">
- Click the button again. The
b
method is called, as expected. Will thea
method be called, even though it is declared asonce
and has logically already been called for this element and event?
d1c57d7
to
6dcaa6f
Compare
cdf9c9e
to
fe95734
Compare
@adrienpoly, you can rebase your branch with upstream master to fix these CI errors: They were resolved in a0fded1. |
5168148
to
293349c
Compare
293349c
to
d76094c
Compare
@sstephenson I have updated the code with all your reviews, there is one comment from Javan that I have not yet added as it is causing some errors in the CI on IE that I don't fully understand right now. I will look into this a bit later I added a test for your edge case. |
e72283f
to
6a7f9ac
Compare
6a7f9ac
to
9e0bbde
Compare
Thank you, @adrienpoly!! |
As per the discussion in this thread, this pull request introduces two new options for events attached to an action
The proposed API is the following
Defaults events
Defaults events with options are not supported
an action like this
must be written like this to pass an option:
considerations:
I see two possible syntaxes to support default event with options
open to discussion....
Tests
I added a new test case for those events in
src/tests/cases/event_options_tests.ts
. The tests foronce
events are ok IMHO but I am having a hard time testing thepassive
eventWhen I test in a real app the passive event I can get it to work but when I log as below an action in my test suite I always get non-passive events.
Will keep investigating, maybe I am missing something here. Could it be that the chrome headless version does not support passive event???
Example
once a dev-build is available (not sure how new build are trigged) here is a test example
https://glitch.com/edit/#!/stimulus-event-options?path=src/controllers/options_controller.js:15:23
Still to do…
handleEvent