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

Update documentation to show no-element-event-actions and no-action-modifiers as related rules #652

Merged
merged 2 commits into from
Feb 11, 2019

Conversation

keanedawg
Copy link
Contributor

@keanedawg keanedawg commented Feb 10, 2019

Both no-element-event-actions and no-action-modifiers essentially do the same thing. I'm adding a note at the bottom of both rules that says they are related.

See:

https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-action-modifiers.md

https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-element-event-actions.md

`no-element-event-actions` and `no-action-modifiers` appear to be duplicates of eachother
`no-actions-modifiers` and `no-element-event-actions` appear to be duplicates
@bmish
Copy link
Member

bmish commented Feb 11, 2019

These rules look like exact opposites. Would love to hear the reasoning behind no-action-modifiers. CC: @bendemboski

@bendemboski
Copy link
Contributor

@bmish I've always strongly preferred inline event handlers, and had the impression that many others do as well, so having a linting rule to help enforce it just made sense.

Does that answer your question, or were you looking to get into a discussion of why I strongly prefer inline event handlers?

@bmish
Copy link
Member

bmish commented Feb 11, 2019

@bendemboski it would be interesting to hear why you prefer them so that we can document the reasoning in the rule documentation.

@bendemboski
Copy link
Contributor

Sure, okay. I guess most basically, it's based on the philosophy that frameworks should identify the pieces of core value they are trying to provide, and for things unrelated to that, they should at most provide the thinnest of wrappers on top of underlying/native functionality even if that leaves warts of the underlying/native functionality exposed. This allows easier adoption because anybody familiar with Javascript/native browser APIs/etc doesn't have to learn a new API/pattern for doing things they already know how to do, easier use by experienced folks because they can maintain simpler mental models of how things work, and easier maintenance of the framework through a smaller API surface area.

Philosophy is great, but let's get practical. Using the distinctions that the Deep Dive article made (and adding one additional one that is key to this discussion), we have four types of listeners:

  1. Ember action listeners on a component's root element
  2. Ember action listeners added via a modifier to some DOM element in a component's or controller's template
  3. DOM event listeners added via on<event>= in a template
  4. DOM event listeners added via EventTarget.addEventListener() from code

And as the article points out, (3) (and (4)) don't play well with (1) and (2) because of the order that events are delivered. Either combination of (1)/(2) or (3)/(4) can cover all of the use cases (that I've ever encountered at least). So, reasons that IMO (3)/(4) wins out over (1)/(2):

  • Easier to learn -- they use a syntax that will be familiar to anybody familiar with writing Javascript to run in browsers
  • Easier to understand/debug -- they deliver the events at exactly the time one familiar with DOM events/bubbling would expect. This is useful for reasoning about code (only one mental model for how/when DOM events are delivered), and for debugging (easier to inject your own event handlers to check if default has been prevented or whatever).
  • Better for 3rd party libraries -- the order of "my" event handlers and 3rd party library event handlers is fully in my control, and in the order I expect. Also, if a 3rd party library triggers a custom event that I want to receive, I don't need to go and add that event name to a list somewhere so Ember will deliver it to me (learning yet another new "API").
  • Easier to maintain -- all refactors are simple and mechanical. If I start/stop needing to access the event object, it doesn't matter because it's always available. Worst-case scenario with (1)/(2) is that I have a type (2) Ember action listener and I start needing to access the event object. Then I either need to switch to using (3)/(4) (changing DOM event deliver order behavior) or write a whole component just so I can switch to (1) and get access to the event object. With (3)/(4) it's a very easy and completely mechanical change to switch between an on<event>=-invoked action and an .addEventListener call.

I might be missing some here because I so rarely use them, but the only ways I can come up with that (1)/(2) win out over (3)/(4) are:

  • In most event listeners, preventing default is either necessary or harmless, and using (2) (but not (1)) removes the need for this sorta-often-boilerplate
  • Passing an event handler to a component to be installed on its root element doesn't require any boilerplate code shuffling to make sure it gets invoked
  • Works consistently with SVG elements (nice, but I believe relatively uncommon use case, and while it sucks that SVGElements don't have an API consistent with HTMLElements, I'm not sure why it would be Ember's job to fix that)

In the balance of things, (3)/(4) seem to me to be the far better choice, especially given that sometimes (e.g. third party libraries), the equivalent of (4) is unavoidable even when trying to only use (1)/(2). Moreover, it seems to me that the (3)/(4) story gets even cleaner with element modifiers (e.g. one could use only (4) if they wanted to, and implement all of the benefits of (1)/(2) in userland via some helpers like register-listener and/or prevent-default/stop-propagation).

@rwjblue
Copy link
Member

rwjblue commented Feb 11, 2019

This convo is great! I'm going to go ahead and merge this PR (I think the rules are clearly related, being that they are generally opposites).

@rwjblue rwjblue merged commit d639b9d into ember-template-lint:master Feb 11, 2019
@rwjblue rwjblue changed the title Documentation Adjustment: no-element-event-actions and no-action-modifiers are now related rules Update documentation to show no-element-event-actions and no-action-modifiers as related rules Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants