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

Add transform for .first() and .get() #37

Closed nlfurniss closed 6 years ago

nlfurniss commented 6 years ago

The code for replacing find(...).first() and find(...).get(x) is almost the same, so decided to combine them into one rule.

Adds a first() and get() transform for Acceptance and Integration Adds tests for both Updates README to include both transforms

simonihmig commented 6 years ago

@nlfurniss thanks for your work here! I merged it already, but a minute later I thought about a possible problem, so reverted that merge for now.

The problem I am thinking of, is that .first() will return a jQuery object, while your transform will transform that to code that return a DOM node, which changes the semantics. I think this can be an issue, when there are some chained jQuery calls following, that this transform does not account for AFAICT.

Example:

find('.foo').first().text()

... will be transformed to...

findAll('.foo')[0].text()

... which is invalid and will break the users code.

The issue does not apply to .get() however, as this already return a native DOM node.

I am not sure how to fix this. One way could be to restrict this when the code pattern is only used in a assert.ok() with no chained jQuery methods following, as in your test fixtures. Or when this is only used in some full code pattern that we already transform, like find('.foo').first().click() -> click(findAll('.foo')[0]). But this seems to be quite some work to get in right, I don't know if that's worth the effort?

nlfurniss commented 6 years ago

@simonihmig, thanks for reviewing!

I don't think this is an issue, because that code example will be transformed in a way that will break anyway.

For example, as the codemod is today, find('.foo').first().text(), is transformed into:

import { find } from 'ember-native-dom-helpers';
find('.foo').first().text()

But at least with first() and last() transformed, those are two fewer manual transform for the user to do herself. I can add something to the readme to acknowledge this if you'd like.

simonihmig commented 6 years ago

@nlfurniss I don't think this is the case, find('.foo').first().text() is left untouched! Just checked this.

One of the prerequisites for this codemod was that it should not create non-sense code. Even when that means that it does not transform everything (which it certainly does not).

nlfurniss commented 6 years ago

Sorry, you're right; I read the jest test results incorrectly 😣

Ok makes sense. I'll break the get transform into a separate branch.

Long term may look at properly transforming chains of jQuery methods so this can land.