ts-migration/no-test-callbacks

[G-Rath]

This is a bit of a tricky one, b/c it's got a bit of "hypothetical" nulls that I can't figure out how to reproduce in action.

As a result, coverage is currently failing, and I've stuck a "please file a github issue" throw as we could use that code to actually test those branches 😂

We could eliminate at least one check by having isFunction narrow to ArrowFunctionExpression rather than ArrowFunctionExpression | FunctionExpression, as only the latter has body defined as being nullable (for reasons I can't figure out - anything I try either has a body or gets thrown out as a syntax error).

Personally, I think they might be typed like that for if you're constructing AST nodes? But I can't find anything solid explanation :/

The other alternative is to use !, but personally I'm not really a fan since it mainly means you'll just have messy errors if it ever is null.

[G-Rath]

@SimenB this might now need rebasing? Not sure why it's got so many extra commits, when it was originally based off reapply-ts about 5 minutes ago.

Let me know what you'd like to do about coverage - this is sort of the POC for a couple of rules that are in this boat.

[SimenB]

@G-Rath I think adding a "please file an issue" is fine if we're unable to figure out why it's null. What I usually do is run the eslint rule over large codebases using Jest, and see if anything explodes. E.g. Jest itself, webpack, babel, lerna etc.. If all is good, I think your current approach is fine.

---

That said, it might be worth an issue in https://github.com/typescript-eslint/typescript-eslint if you're unable to to reproduce what the type info tells you. Type comes from https://github.com/typescript-eslint/typescript-eslint/blob/807bc2d4f5dbd62aebfcbd01012d41c37066480f/packages/typescript-estree/src/ts-estree/ts-estree.ts#L485

[G-Rath]

@SimenB Awesome. Do you have any secret tips on how to quickly test out the new code? I imagine copying & pasting into node_modules should be enough.

[SimenB]

I do yarn link as it's easier to make adjustments that way. However, copying over the rule and the utils should work (as long as the other repo also has @typescript-eslint/experimental-utils, which at least jest does)

[SimenB]

I've rebased this. Are you able to verify the body thing we discussed and either add a test here or an issue at the TS eslint repo?

[G-Rath]

I tried to test it out against one of my repos, but for some reason eslint kept erroring with Cannot read property 'meta' of undefined.

My IDE somehow was able to run it just fine (which I double checked a couple of times w/ some throw new Errors).

The rule worked fine, w/ this code being reported:

test('some test', done => {
  expect(false).toBe(true);
  done();
});

and "correctly" fixed:

test('some test', () => {return new Promise(done => {
  expect(false).toBe(true);
  done();
})});

The fixed code violates jest/no-test-return-statement, but we can worry about that another time.

For now, I think I'm just going to open up an issue on the TSEslint repo :)

[SimenB]

Seems like I'll need to investigate #268 then...

[G-Rath]

I can't confirm that my repo was using the latest code - I just fired from the hip by checking out this branch & copying the lib after running yarn build into node_modules.

I played around w/ updating a few packages on my repo just to be sure.

You can see the project structure I use in this repo, just in case it matters that I'm using an extending .eslintrc :)

[SimenB]

Cool thanks, pushed a fix to reapply-ts 🙂