simonihmig / ember-native-dom-helpers-codemod

Codemod to transform your jQuery based ember tests to use ember-native-dom-helpers
MIT License
27 stars 9 forks source link

Fix another edge case with single-line arrow functions. #12

Closed cibernox closed 7 years ago

cibernox commented 7 years ago

This demonstrate a real case that fails, along with what I think would be the ideal generated code.

Ember.run(() => this.$('select').val('1').trigger('change')) seems to break. I think that it's because the transformation tries to transform it into

Ember.run(() => 
  await fillIn(...)
  await triggerEvent('change');
)

but arrow function without braces are expressions and that's invalid.

Since wrapping this kind of interactions in Ember.run is not needed, we could detect that and remove it.

simonihmig commented 7 years ago

Hm, yeah... those edge cases...

I don't know, I don't feel very strongly about supporting this, do you? I guess it could become quite complicated, and the codemod could end up doing really wrong things. Just think about the (optional) other parameters of Ember.run (like target and args). I guess we could easily screw things up there, for some even "edgier" edge cases?

Right now, it seems it just refuses to transform things, so there will be some jQuery left overs that users would have to manually convert. When doing this for ember-bootstrap, there was still quite some code that did not get converted automatically, but the majority (the really common cases) did. Acceptable from my POV, at least better than creating really wrong code...

I don't know, what do you thing?

cibernox commented 7 years ago

@simonihmig I agree that I don't care too much about those edge cases, but if a transformation fails, the entire file is skipped I think. I think that we can leave those edge cases untransformed but make the transformations resilient to failure.

simonihmig commented 7 years ago

That's a valid point!

cibernox commented 7 years ago

@simonihmig I think I almost have it. Hold on.

cibernox commented 7 years ago

@simonihmig Fixed. I basically don't attempt to apply the transforms in set-value.js on single-line arrow functions.

cibernox commented 7 years ago

@simonihmig I changed that to only try to access the grand grand parent of the rest of the checks pass. If all check pass, we know at least that the grand grand parent exist, even if it's the top-level program.

simonihmig commented 7 years ago

Merged! 👍