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

Possible improvements #1

Closed cibernox closed 7 years ago

cibernox commented 7 years ago

I'm on mobile so I haven't tried any of this, but I've seen a couple improvements:

This is all I've seen on the readme. Once I'm back from holidays I'll try it myself.

simonihmig commented 7 years ago

this.$('.foo').attr('id') => find('.foo').id. I think that it's more idiomatic.

@cibernox That's a source of constant confusion I think, in what is a DOM node attribute and what a property, at least for me. And certain attributes are so called 'reflected properties', like id, that can be used as properties as well (see http://stackoverflow.com/a/6004028). So I don't know how to best handle this. id still is primary an attribute, so using getAttribute seems perfectly valid, but as a reflected property, using .id would work as well. But that applies to many attributes, so to be consistent, should we use the property syntax for all reflected properties?

This would require maintaining a list of all reflected properties, and using that to convert all instances of .attr('reflectedProp') to .reflectedProp, instead of .getAttribute('reflectedProp'). Right now the conversion is pretty straightforward and simple: .attr('foo') -> .getAttribute('foo') and .prop('bar') -> .bar, without any knowledge of what foo and bar really are (attr or prop).

I would think the current approach is "good enough", but open for other thoughts...

Regarding the two other transforms you mentioned: good to know, will try to change them!

If you've got any other suggestions, would be happy to fix them! :)

cibernox commented 7 years ago

Yeah, I'm not very concerned, but I thought that id and class are so prevalent that it might worth treating them differently and create less verbose code.

As a general rule I think that your approach of .attr() -> getAttribute() and .prop('foo') -> .foo is correct..

But ids and classes easily account for for 90% of the times I check for attributes of an element, that's why I think that we could make an exception for those two.

In particular, I'm thinking in accessing the id as a property instead of an attribute, because they match, and also being smart enough to replace $el.hasClass('foobar') by classList.contains('foobar').

Again, I wouldn't push this very hard if you think it doesn't worth the effort, I just thought that it would be closer to the code I'd write if I did the transformation manually.

simonihmig commented 7 years ago

Ok, I will see, shouldn't be that hard. I was a bit hesitant to make this work for all possible cases of reflected properties, as this would require maintaining and coupling this very strongly to the HTML spec. But treating id as a special case here probably makes sense...

simonihmig commented 7 years ago

Your proposed improvements are implemented now! :)