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).

[Turbo87]

I tend to agree. I think the safest option would be to use a CLI flag that is off by default and explained in the README.

[simonihmig]

Thanks for your views folks, with which I strongly disagree! 😜

I am probably opening a can of worms here, and what follows is not very specific to the problem at hand here, but more of a general nature. But I am asking myself what kind of role a codemod should play.

Possibility A: a codemod should strive for correctness, i.e. do transformations of code that result in another form of code that is equivalent to the input. Or in other words that does not break things.

That sounds good at first, but without going into CS-y terms like NP-completeness it is still fair to say that correctness is a noble goal at best here. I have yet to see a codemod that comes even close (unless it is doing just micro-syntax like transformations). E.g. the native class codemod run on a larger codebase of ours broke so many things in so many ways (admittedly quite some time ago).

On the other hand there is possibility B: a codemod should just provide value to its user, by doing the kind of code transformations that it can reasonably do and are useful, while leaving those that require a human brain to the user.

I am obviously in this camp.

To back this up: when I ran this codemod on ember-bootstrap's codebase, it skipped over ~95% of the components. Those that it did transform were so trivial, that I could have done those probably in only slightly more time as running npx tagless-ember-components-codemod took. So basically the codemod did not provide any value, at least in my case.

So to get value out of it, this one and a couple of other PRs were required, and I had to comment out this line locally to not skip when this.element is used. Obviously this left the codebase in a broken state after the codemod, but still I was able to concentrate on those tasks like eliminating this.element usages, but not have to uselessly spend time on that purely mechanical work that the codemod could do (e.g. moving the root element markup including classNameBindings and whatelse into the template).

Further on the point of correctness: as said above this codemod skips over components that call this.element as it does not know how to transform that usage automatically (and cannot). But it does not do this when one of the following is used:

• const element = this.get('element');
• const { element } = this.getProperties('element');
• const element = this.getWithDefault('element', document.body);

You could probably consider this a bug. But what about this:

const element1 = this['element'];
const element2 = this['tnemele'.split('').reverse().join('')];

Would you want to write this? Hopefully not. Is it valid JS? Certainly. Would a codemod not detecting this and thereby breaking your code be considered incorrect in a strict sense? I think so!

My point is, to prioritize bringing value to the user than to strive for correctness. I am not saying the latter is unimportant, but that the former is more important, IMO!

@chriskrycho to your point of Ember events: yes, I understand, and I believe I am pretty much aware of the differences. But as you also know there is no other way than to deal with those native events with {{on}} when going to outerHTML semantics. So what's the point of forcing the user to do the code transformations manually, when (s)he will eventually run into those differences anyway? That does not depend on whether a codemod was used or not.

Adding the ones which might break stuff in edge cases behind a feature flag is a good compromise in my opinion.

I am ok with that in general. In this case I would probably prefer a general flag like strict mode vs. loose mode, to allow for more optional transformations. But tbh, as I tried to explain above, I don't expect that strict mode (the default) will provide a lot of practical value.

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).

Exactly. You cannot transform the class without migrating event handlers, and you also cannot migrate event handlers without moving the component's "root" element into the template. So all of that has to happen in one transformation. I cannot see how separate codemods could do this.

Sorry for the long ramblings here! 😝 Have a nice weekend, and stay safe! 😀