Support event handlers

[simonihmig]

In the current form, it seems to me the codemod will skip over a lot of components, as many will have at least one event handler like click().

I believe it should be possible to transform these:

import Component from '@ember/component';

export default class FooComponent extends Component {
  tagName = 'button';
  click() {
    // do something
  }
}

{{@text}}

will be migrated to:

import Component from '@ember/component';
import { action } from '@ember/object';

export default class FooComponent extends Component {
  tagName = '';
  @action  
  handleClick() {
    // do something
  }
}

<button {{on "click" this.handleClick}}>
  {{@text}}
</button>

Or do I miss something that would make that more complicated?

Btw, this is assuming we have #3, don't know how useful this is for classic classes. 🤔

[Turbo87]

it might be possible to do this, yes. but it does raise the complexity of this codemod significantly. It also still does not solve the second issue in https://github.com/ember-codemods/tagless-ember-components-codemod#known-caveats, so it still has similar caveats.

[jelhan]

It also still does not solve the second issue in https://github.com/ember-codemods/tagless-ember-components-codemod#known-caveats, so it still has similar caveats.

But these are two different concerns in my opinions. Being able to automatically convert components to outerHTML is one thing, handling breaking changes that these may introduce is another one. There are a lot of changes that may be considered breaking depending on what is considered part of the public API if converting a component to outerHTML...

[simonihmig]

I agree. For one thing IMHO this is pretty much an anti-pattern, to set/override an event handler from outside. But more importantly, you will always have to fix that "API" usage when refactoring to tag-less components (and eventually later to Glimmer, as I see this codemod mainly as a stepping stone for, right?), irregardless of how you manage the transformation (with the codemod, or manually). So this is a general "problem" with tag-less components, and not a particular concern of the codemod.

[chriskrycho]

@simonihmig @jelhan @Turbo87 I've been mulling on this one as I put together out guidance internally for the app I work on, and as @lbdm44 and I dug into it, we concluded it's probably best not to do it, for one specific reason: the built-in component event handlers fire Ember events, while {{on}} fires native events. (For anyone who reads this and isn't familiar with the distinction, @mariechatfield's EmberConf 2018: Deep Dive on Ember Events covers the differences nicely.) At most it should be an optional transform, and I'm currently inclined to say it should probably just be its own codemod.

[jelhan]

I think the difference only matters in a very few cases. This codemod has already proven it's value when migrating the huge code base of Ember Bootstrap. It's way better to have a codemod that migrates 95% of the cases correctly and fixing the other 5% by hand than needing to do everything manually in my opinion.

But I get the argument that codemods should only do safe transforms by default. Not everyone has a good test coverage.

Adding the ones which might break stuff in edge cases behind a feature flag is a good compromise in my opinion. Splitting it over several different repos doesn't help as you might need a combination of risky features to migrate a single component (e.g. event handlers and class name bindings).