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

{{on}} Modifier #471

Merged
merged 5 commits into from
Apr 12, 2019
Merged

{{on}} Modifier #471

merged 5 commits into from
Apr 12, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Mar 22, 2019

text/0000-on-modifier.md Outdated Show resolved Hide resolved
text/0000-on-modifier.md Outdated Show resolved Hide resolved
text/0000-on-modifier.md Outdated Show resolved Hide resolved
text/0000-on-modifier.md Outdated Show resolved Hide resolved
@rtablada
Copy link
Contributor

@pzuraq something that should be clarified in the design is that the on modifier always passes the browser event to the called function. This is different than action (used as a modifier) which does not pass in the event and is still slightly different than on*={{action}} invocations.

text/0000-on-modifier.md Outdated Show resolved Hide resolved
text/0000-on-modifier.md Outdated Show resolved Hide resolved
@luxzeitlos
Copy link

shouldnt this deprecate the {{action modifier? will there still be a use-case for it?

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 22, 2019

We generally don’t introduce new features and deprecate old features in the same RFC, so we don’t bikeshed unnecessarily about the deprecation when its a somewhat orthogonal question.

@richard-viney
Copy link
Contributor

v0.8 of the ember-on-modifier addon recently added support for passing through positional args to the function: https://github.com/buschtoens/ember-on-modifier/releases/tag/v0.8.0

This is convenient as it alleviated the need to use the bind or action helper, but I'm assuming this feature is intentionally absent from this PR? Does anyone know why it was added to ember-on-modifier, e.g. as an experiment, or by user request, etc. @buschtoens

@richard-viney richard-viney mentioned this pull request Mar 23, 2019
@Serabe
Copy link
Member

Serabe commented Mar 23, 2019

@cibernox
Copy link
Contributor

I want to bring one topic. When the passive listeners were added to browsers the dev relations in Google like @jakearchibald commented that if event listeners were designed today, passive=true should be the default as its better for performance and in most cases you don't need active listeners.
Iirc, chrome experimented with changing the default but the backslash was too big and they reverted it.

As this is a new API perhaps we can afford to change the default in order to be performant by default.

@rtablada
Copy link
Contributor

@cibernox I would say that we would not have any hash params set by default. This way we don't have to chase browsers default params (or have inconsistent behavior to what users would expect). passive=true has a bit back and forth historically and has been undone in some cases and event types because of unexpected behavior.

@rtablada
Copy link
Contributor

@richard-viney that should probably be added to the alternatives section. In initial design for this RFC it was discussed that we would favor composition instead of increasing the surface of on and bind to avoid some of the complexity of action. While on "click" (bind this.foo arg1) is a bit more to type it is clearer and easier to reason about than remembering "positional params after the function get applied as arguments at call time"

function, users should use a helper such as the [bind helper](https://github.com/emberjs/rfcs/pull/470):

```hbs
<div {{on "click" (bind this.addNumber 123)}}></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how well this correlates to JS semantics

@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 24, 2019

We discussed this RFC a bit at the f2f on Friday, one concern that was brought up by @rwjblue and needs to be addressed was the interop story with the {{action}} modifier. The way the {{on}} modifier works currently, it would have the same pitfalls as using onclick= properties, where event bubbling would be out of sync between the two. This could make it difficult for codebases to adopt the new modifier progressively, over time.

One option would possibly be to add a flag that causes {{on}} to integrate into the event delegation of action - that is, by enabling the flag, event listeners would be added to the global event listener instead of the local one. When all {{action}}s had finally been converted to {{on}} in a codebase, the flag could be flipped back, and seamlessly convert the codebase to standard browser event delegation. I think this strategy would become an issue with event handlers in classic components though - they likely can't be converted as easily to standard bubbling, and flipping the flag could result in bugs.

The other option is to explicitly not support any interop, and instead focus on guides helping users to debug and convert forward toward {{on}}. This seems less than ideal though, since it would mean the ecosystem can't easily transition without worrying about breaking changes.

@chriskrycho
Copy link
Contributor

@pzuraq:

the interop story with the {{action}} modifier… would have the same pitfalls as using onclick= properties, where event bubbling would be out of sync between the two.…

The other option is to explicitly not support any interop, and instead focus on guides helping users to debug and convert forward toward {{on}}. This seems less than ideal though, since it would mean the ecosystem can't easily transition without worrying about breaking changes.

Can you elaborate on this? I'm not seeing how the isn't an inherent cost of transitioning the ecosystem away from the many degrees of magic in {{action ...}} but it's possible I'm missing something.

Here's what I'm thinking:

As long as there is a clear migration path for each current usage of {{action ...}}, users can migrate at their own pace from the one to the other unless or until {{action ...}} is first deprecated and then removed. And the issues with {{on ...}} and {{action ...}} here are the same as the existing issues with event handling and {{action ...}}, if I understand correctly, so the situation is not different in practice… except that we'll have clear guides on how to solve those issues!

What's important is that there be a clear upgrade path and a set of well-documented paths for each way people are using {{action ...}}, and if or when {{action ...}} is deprecated, we should make sure that we make distinct deprecation warnings and docs for those (if possible). While that might eventually make a cliff at some cutoff point (Ember 4.0 or 5.0 or whatever), that's true of any deprecated code path.

I'm strongly in favor of the simpler (and more correct!) model here.


Here's why I'm thinking that:

I tend to think "introduce the breaking API with a clear migration path and no deprecation of the old one" is the better path. It's possible to over-optimize for "smooth transition," usually by introducing additional framework complexity. Introducing that additional complexity to make a smoother path in initial transition seems—

  • very likely to introduce its own source of edge cases (not to say bugs 😄);
  • certain to create technical costs in the form of extra code paths to maintain and test, which will have to be paid down later;
  • and still likely to introduce churn, just at a later point in the adoption curve.

Given that the point is to get rid of the problems (including with event delegation!) from {{action ...}}, attempting to make it easy to interoperate two incommensurable systems does not seem worth it to me.

@buschtoens
Copy link
Contributor

buschtoens commented Mar 29, 2019

During the development of ember-on-modifier I found out that IE11 a) does of course not support { once: true } and b) also tends to throw unexpected errors, when given a third parameter for addEventListener. Because of this, and because not supporting once for IE11 was not an option, I did this: ember-polyfills/ember-on-modifier#2

@locks locks added T-framework RFCs that impact the ember.js library T-templates labels Apr 2, 2019
Copy link
Contributor

@chrisrng chrisrng left a comment

Choose a reason for hiding this comment

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

Should we update the bind helper reference to with-args based on updates to that RFC?

text/0000-on-modifier.md Outdated Show resolved Hide resolved
text/0000-on-modifier.md Outdated Show resolved Hide resolved
pzuraq and others added 2 commits April 3, 2019 08:52
Co-Authored-By: pzuraq <me@pzuraq.com>
@rwjblue
Copy link
Member

rwjblue commented Apr 5, 2019

The core team met today and decided to move this into final comment period.

@pzuraq pzuraq self-assigned this Apr 5, 2019
@rwjblue rwjblue merged commit ce65744 into master Apr 12, 2019
@rwjblue rwjblue deleted the on-modifier branch April 12, 2019 18:57

1. The event name as a string as the first positional parameter
2. The event listener function as the second positional parameter
3. Named parameters as options
Copy link
Contributor

@buschtoens buschtoens Apr 13, 2019

Choose a reason for hiding this comment

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

This is kinda late and I apologize for that, but what will be the behavior, if the event name or event listener are missing or have the wrong type? ember-on-modifier currently checks for typeof eventName === 'string' && typeof callback === 'function' and only then registers the listener. If the check fails, no error will be raised. We might want to make this a hard assertion instead.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I believe that we will validate name and listener via assertions. I suppose we could do something to allow null/undefined but it really seems more likely to be a bug than intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

@richard-viney One solution would be using (optional) from ember-composable-helpers, which returns a no-op function, if the input is not a function:

<button {{on "click" (optional (if this.isListening this.listener))}}>
  ...
</button>

While this requires extra gymnastics, I agree with @pzuraq and @rwjblue, that it is probably safer to assert against missing eventName and callback to prevent accidental errors.

@richard-viney
Copy link
Contributor

richard-viney commented Apr 15, 2019 via email

@richard-viney
Copy link
Contributor

richard-viney commented Apr 15, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.