Add tail eq support

[DingoEatingFuzz]

I had a bunch of these in my tests and I didn't want to deal with them by hand.

To be clear, this only handles the case where the :eq directive appears at the end of the selector. No support for :eq in the middle, no support for :eq in a template string, and no support for :eq in a variable that is later passed to a method.

Still though, this was useful for me and probably others.

[simonihmig]

This will close #8 (with the mentioned restriction that it only handles :eq at the end, but I guess that covers most actual usage patterns!?). @cibernox we talked about this missing feature back then, so mind having a quick look?

[DingoEatingFuzz]

What about changing the API of addImportStatement in that we don't have to pass root here and just collect all additions in a private array, and have another writeImportStatement(j, root) at the end of each transform, that actually writes the new AST nodes from that array. Does that make sense?

Definitely less invasive, so a win on that front. Still a little weird in that now the utils file becomes stateful.

[simonihmig]

Still a little weird in that now the utils file becomes stateful.

Well yes, was my first thought as well. But addImportStatement is now also not side-effect free, it is changing state, but just somewhere else (directly mutating the AST tree), right? ;)

[cibernox]

Wrong comment. I thought I was on another repo.

[cibernox]

There is mainly on bug with the implementation (findAll('.klass')[0].length is not valid. I understand that is a PITA tho. I know that it's impossible to account for all possible .whatever, but .length is the most used property called so it might deserve some love.

Also, and this is a totally optional suggestion, :eq(0) could be transformed into find instead of findAll()[0]

[simonihmig]

Wrong comment. I thought I was on another repo.

Oh man. I was only looking at my email notification of your initial comment and was rereading that three times and still not getting it... 😝

[cibernox]

I think that github should let the comment settle for a couple minutes before emailing it. I can't count the number of times I edited a comment just after making it. 4 times.

[simonihmig]

findAll('.klass')[0].length is not valid.

Good catch! This case is even in the new test fixtures, makes no sense to assert for invalid code: https://github.com/simonihmig/ember-native-dom-helpers-codemod/pull/23/files#diff-86046aa0f01ab7db6af2be674456f7c5R36

[DingoEatingFuzz]

Thank you both for the prompt reviews. I made some changes.

1. Imports to write are tracked in a private set, as discussed. This ended up still being a wide changeset, but at least there is less reference passing.
2. The :eq(0) case is special cased. 2.1 In certain cases, redundant function calls are compacted (e.g., click(find('.foo'))).
3. When the findAll selector is migrated in a manner that results in code different than a straight findAll call (e.g., findAll('.foo')[2]), it is wrapped in an array.

A note about point 3: I think we can all agree that the output code can be nonsense, such as [ findAll('.something')[5] ].length. That will always be 1, but the original code was also nonsense, and at least this won't have runtime errors.

[cibernox]

@DingoEatingFuzz The problem is that if findAll('.foo') has 2 results, and you end up having [findAll('.foo')[4]].length, you are doing [undefined].length, which would incorrectly seem like the element was found when it wasn't.

IDK how complex would be to specifically recognize .length as a special case in expressions like this.$('.selector:eq(2)').length, but I'd rather generate an expression that has a runtime error that then users will immediately find and fix themselves than generating code that makes no sense and can lead to hidden errors.

[DingoEatingFuzz]

What expression are you expecting? The more I think about this, the more I am certain there is no good option.

[cibernox]

@DingoEatingFuzz I also think that there is no perfect option, so I'd rather generate findAll('.thing')[4].length and let it fail rather than doing something like [findAll('.thing')[4]].length.

In some cases, inside assertions like assert.ok(this.$('.thing:eq(4)').length) you could guess that the perfect transformation would be assert.ok(findAll('.thing')[4]), but that requires context of wether or not you're inside an assertion and its type, which is probably overkill.

I'd say: let it fail.

[DingoEatingFuzz]

👍 I just removed the commit that did the array wrapping, so now it will generate code like findAll('.thing')[4].length.

[cibernox]

I have no more suggestions. @simonihmig if you don't have anything else, I think this is good to go.

[simonihmig]

Published as 0.2.0, thanks again @DingoEatingFuzz!