Allow awaited expects to be next to regular ones

[tmercswims]

Full disclosure: This is my first foray into the world of ESTrees, so this solution comes largely from a place of "well, I found a way to do it that works" and less one of "I know what I need to do and I did it."

So that said, if there is a completely different approach that you had in mind when first reading the issue I filed, then I'd be happy to re-approach this, or hand it off; whatever you prefer.

Fixes #98.

[dangreenisrael]

What is this being used for? I wasn't able to find CallExpression anywhere in the code.

[tmercswims]

It's the value of node.expression.argument.type when node.expression.type is 'AwaitExpression'. It gets passed up to createTokenTester() and used to verify that the node there is the type that we expect it to be.

Sadly I can't really give a better explanation than that - this is just what I discovered empirically by stepping through the code as it was running into an expression like await expect(...)....

[benkimpel]

re: CallExpression

I think that we'll always get a CallExpression in our particular case with await expect(...), but there's definitely more to AwaitExpression's argument than that. Here's the interface for an AwaitExpression. The argument is an Expression, such as CallExpression, MemberExpression, etc.

@tmercswims How would you feel about handling AwaitExpressions more generically in the original createTokenTester? Then we wouldn't have this additional tester just for expect. Maybe something like...

# pseudo-code

if expression statement
  if await expression
    node = node.expression.argument
  end
  
  token = get first token from node
  return token is identifier and is a match for token value
else
  return false

I think something like that above would provide a more general fix, because someone could always do something like this...

async function whatHaveIDone() {
  // These describes are converted to resolved promises and are a valid usage of await...
  await describe("why have i done this", () => {});
  await describe("i am a monster", () => {});
}

whatHaveIDone();

This would mean there's no particular check for CallExpression although that is almost always what we would get. We'd just essentially be saying "Oh, it's an await... I need to look at the next node for the first token" in all cases, not just for expect.

[tmercswims]

Absolutely makes sense, and something I probably should have thought to refine 😅 I've done the change pretty much exactly how you describe, and it works great.

[dangreenisrael]

Thanks for the quick PR 🙏

[benkimpel]

@tmercswims @dangreenisrael This is great! Mind if I take a more careful look tomorrow during my free time?

[tmercswims]

Fine with me of course, take your time! 🙂

[benkimpel]

@tmercswims @dangreenisrael A small suggestion for you to consider. Let me know your thoughts. Not a showstopper, but worth a look.

@tmercswims Thanks for the PR. It's very close to merge, imo.

[benkimpel]

Absolutely makes sense, and something I probably should have thought to refine 😅 I've done the change pretty much exactly how you describe, and it works great.

@tmercswims Awesome! Glad it worked out. Thanks for the bug report and fix.

@dangreenisrael I approved PR, but I've got two questions for you related to merge: 1) How do you want to handle versioning on this? It's a bug fix for us, but it could break CI runs for our users w/o any code changes on their part. Not sure of the standard versioning approach for a linter, honestly. 2) Shall we squash these commits down to one?

[dangreenisrael]

1. Excellent point. On the one hand, it is a bug fix. On the other hand it is an incompatible change, this will require a code change for most users.

I tend to think that that would actually make it a major change, even though it is a bug fix. If you guys feel weird about that, another option would be to add this optional and opt in - but I think I am leaning towards major version change.

2. Squash and merge please. I am going to disable non-linear git history in the settings.

[tmercswims]

In my opinion a major version bump is the right way to go, since this definitely does have the potential to start calling out things that it didn't before. Putting it behind an option feels strange, since it seems like the sort of thing that should "just work."

Just my two cents.

[dangreenisrael]

@benkimpel any thoughts?

[benkimpel]

Major bump makes sense to me, too.

[dangreenisrael]

Sounds good. @tmercswims do you mind bumping the version in the package.json and updating the CHANGELOG?

[benkimpel]

Sounds good. @tmercswims do you mind bumping the version in the package.json and updating the CHANGELOG?

@tmercswims If it's not to late would you mind also squashing down to one commit with a commit message in roughly this format. We don't use conventional commit on this project, although that's possible some day.

[tmercswims]

Done and done. I went ahead and dated the version May 4th, in anticipation of it happening today, but I can always change it if it doesn't. I can also adjust the actual change note if you guys had something else in mind.

I also wasn't sure if more description was really necessary in the commit as it's pretty self-explanatory with just the subject, so I settled on nothing - if you would like me to add more detail there, I'd be happy to.

[benkimpel]

@tmercswims thanks