Updating `migrateSelector` to support selectors as variables

[scalvert]

Adding a failing test for #27

[simonihmig]

Thanks again! Would you mind moving these changes to the jq-extensions.(input|output).js fixture files, as this is where we check for those jQuery specific selectors? Does not make a real difference, just helps to keep things in order! ;)

[scalvert]

The failing tests are moved. I noticed that we'll likely have a problem if constants are imported and used within that file. It may be better to opt out of transforming in that case.

[scalvert]

Adding more test cases, this identifies some more challenges.

When a pseudo selector that contains a transformable is stored in a variable, it can potentially create invalid calls.

const NORMAL_PSEUDO_SELECTOR = '.foo:eq(0)';

Under normal circumstances, if this was a bare selector, we'd transform it to this:

  assert.ok(findAll(find('.foo')).length);

But because this is stored in a variable, and if we attempt to lookup the variable's value in order to determine whether we can transform it or not, it's going to ultimately be invalid. This is because '.foo:eq(0)' is unpacked to .foo, which in this case we can't do as it's inside a variable reference.

Any thoughts on how to handle this, @simonihmig, @rwjblue?

[scalvert]

OK I updated this PR to include a potential fix for this issue. This fix (discussed with @rwjblue offline) proposes leaving selectors as identifiers alone, and simply referencing them via array indexing. This allows us to partially convert to using await and click, but not fully remove jQuery calls.

The result of this looks somewhat like the following:

await click(findAll(this.$(selector)[0]));

The alternative approach, where we attempt to reference the variables contents and inspect it, is a rabbit hole of permutations. You'd need to inspect the variable's contents, then subsequently determine if it's a DOM API compliant string. This is further complicated by the fact that we attempt to transform jQuery-like selectors in to their associated DOM equivalents. Going this route would add additional branching and complexity that I'm not sure is warranted for this codemod.

I'm open to discussions on alternatives.

[simonihmig]

@scalvert first of all thanks for working on this!

The alternative approach, where we attempt to reference the variables contents and inspect it, is a rabbit hole of permutations.

I was working on this in parallel, and I think I have something working. It's using the scope functionality to check the variable content. I still need to refactor a few things to make this working everywhere. Will try to do this tomorrow (it's already late over here) and create a new PR, we can discuss then...

[scalvert]

No problem. I'm totally open to other options, but this felt like the least risky option to pursue.

[rwjblue]

I think #29 does a good job of detecting when a local variable can still be used with the native helpers, but does not address the case where we can definitely not use native helpers with the value in the variable directly. Just off the top of my head this would include imported selectors, selectors that include non-transformable jQuery extensions, etc. In these cases, I still think the right approach is to use click(this.$(WHATEVER_THING)[0]). Migrating to the native DOM helpers is still a valuable thing to do even if some jQuery usage is still involved...

tl;dr @scalvert - Can you rebase this on top of #29?

[scalvert]

So I think there's more work to be done here. It's likely we need a combination of @simonihmig's work, and some additional work on making selectors migrate when they're incompatible with native DOM.

I think we should publish #29 as is, and we can iterate on this after.

@simonihmig, thoughts?

[simonihmig]

@scalvert Happy to release this. But can you elaborate on the additional work, is there something you noticed we haven't talked about yet? I was thinking that the changes you prepared here (with the exception of the .get() thing, but which should be easy to change) would mostly cover what's needed, but I might miss something...

[simonihmig]

I think we should publish #29 as is

Done, as 0.2.3!

[scalvert]

Sorry for the delay - I was out of town for a few days.

Specifically, the changes I made don't cover everything we want to address. There are some specific cases that we don't want to miss. I'm going to work on capturing these cases and will update this branch with the list.