Rewrite in TypeScript #256

SimenB · 2019-05-10T17:32:41Z

Now that typescript-eslint/typescript-eslint#425 has been released, it should be relatively straightforward to migrate.

I think porting one and one rule makes the most sense. Feel free to pick one 🙂

The text was updated successfully, but these errors were encountered:

G-Rath · 2019-05-26T09:46:11Z

I've done a first pass of no-mocks-import, which passes all the tests.

So far, it seems to me like this'll be a bit teeth pully, as ts-estree doesn't provide you with lots of nice type guards like TS does - as a result, I've started to make my own in tsUtils, but am trying to be conservative for now, since I don't know ts-estree well enough to predict how wild things might get.

I've also encapsulated the actual primary condition into a isMockImportLiteral function - let me know if you're happy with how I've done that; I've never worked in the FB/jest domain, so I don't know what you guys prefer for coding in general 😂

Finally, off the bat there's already room for improvement, which ties in to everything I've said so far: right now the rule is only checking actual string literals, which means template strings already won't work right off the bat (regardless of if they've got actual embedded expressions or not)

This could be improved to support them w/o expressions, but you could also take this further to try and provide basic support for expressions, since you might be able to do a basic search of surrounding nodes in an attempt to see if the expression is a constant, and if it is evaluate its value.

All in all, for now I've opted for doing the basics, w/ the idea that people can fill issues requesting improvements, just to keep things simple.

I've not made a PR since I saw your previous PRs were reverted following some breaking of peoples builds. Let me know if you're still wanting to go ahead with converting to TS.

For now I'm going to continue trying to pick off the easy rules - let me know any thoughts you have :)

jeysal · 2019-05-27T09:21:17Z

@G-Rath thanks for investing time in this! What you've described sounds fine to me. @SimenB will probably want to kick off a second attempt at TS migration at a less busy time

SimenB · 2019-05-28T09:29:30Z

Thanks @G-Rath! Yeah, it seems using the module from typescript-eslint/typescript-eslint#425 requires consumers to use typescript as well, so I need to figure out how we can author rules in TS without consumers noticing at all. I'll try to get to this, but I'm still swamped at work... Feel free to dig into it if you don't mind!

G-Rath · 2019-05-29T02:00:56Z

I don't know enough about the eslint ecosystem to know if its worth using the @typescript-eslint/parser over the current one, which is now my question - what is the advantage?

I'm happy to help write TS, regardless of what it's for, but yeah I think the decision that needs to be made is about converting to TS vs switching to the @typescript-eslint/parser.

The former could be started on right now if you wanted, but as you've found out the latter is a breaking change, and thus comes down to pros vs cons which I obviously can't decide for you :)

SimenB · 2019-06-03T16:14:22Z

I don't know enough about the eslint ecosystem to know if its worth using the @typescript-eslint/parser over the current one, which is now my question - what is the advantage?

We don't want to use the parser in our own rules - we want to use the utils which provide way better types than the ones from DefinitelyTyped as well as some nice utils for options etc. It's also stricter (also called opinionated 😛), forcing us to follow some good conventions (fixing both #201 and #202 at once).

We do however want to use the parser for linting our own code, but that should not be visible to consumers.

The dependency on typescript was not on purpose, see typescript-eslint/typescript-eslint#425 (comment)

When a release is out, we can probably retry this (I'll revert my earlier revert). Will need to dig into #268 as well at that point.

Mark1626 · 2019-06-23T14:37:24Z

@SimenB I'll pick up a rule, can you tell me where code migrated to typescript is maintained.

SimenB · 2019-07-17T13:27:59Z

A new try is up here: #305.

@M4rk9696 thanks! A PR to that branch (reapply-ts) is most welcome. Or if I'm able to land it, to master

SimenB · 2019-07-18T20:53:33Z

@G-Rath would you be able to PR no-mocks-import to the reapply-ts branch of this repo? 🙂

G-Rath · 2019-07-18T21:00:14Z

@SimenB I can but try ;P

Give me a few minutes (or an hour, depending on the work - I've not actually been following the activity 😬)

SimenB · 2019-07-18T21:00:34Z

There is absolutely no rush 🙂

G-Rath · 2019-07-18T21:45:57Z

@SimenB done in #309

It took a bit longer than a few minutes b/c of a standup meeting & having to update the code a bit (but nothing major).

All the tests are passing, so I think it's fine :)

G-Rath · 2019-07-19T07:08:39Z

I've been doing everything in a single generic branch, and have about a quarter of the rules converted.

My plan is to just cherry-pick at the end. There are a couple of that don't require tsUtils that I can cherry-pick, which I'll try and do now.

Otherwise, I'll finish off the rest off the rules this evening/weekend :)

Sorry for all the failing tests - I'll check them out once I'm home, as I'm rushing to catch a train 😬
I have no idea what's up w/ the one about the git reference, and so don't know how to fix it either.
I'm pretty sure it's related to my fork & updating it? Either way, the code is there.

SimenB · 2019-07-19T07:37:52Z

Oh wow @G-Rath, that's awesome!

Don't worry about the commitlint check on CI - it's confused by the merge commit travis creates. As long as lint and test (with coverage) passes, you're good 🙂

G-Rath · 2019-07-19T08:37:09Z

@SimenB just a heads up that the meta property should be checked on all my PRs; I've been forgoing trying to figure out what the best values for the description, setting the correct "recommended" value, the "category", and the "type" in favor of converting the actual rules as I feel those are somewhat arbitrary.

I've been just copying the header for each rule as the description :)

I've also not tested any of the fixes - I think they'll work fine but the only thing I'm worry about is what happens if you return an empty array..? I've not written eslint plugins before so don't know what the best practices are.

I'm happy to follow your lead on any changes you want made.

SimenB · 2019-07-19T08:41:22Z

Ah, ok, good to know. I'll verify

The fixes are all covered by unit tests, so if the tests pass then we're good. If a violation is not fixable, we should not return a fixer (that way the "n error potentially fixable" message from eslint is not misleading)

G-Rath · 2019-07-20T10:56:08Z

I think we've nearly gotten all the low hanging fruit, so I'm going to look to tackle the converting of the exportCase & other type guard util functions (like method, method2, etc).

I've actually already done exportCase, but I've not pushed it yet b/c I need to explore them a bit more, as some of them are definitely guards, while others are not.

Once that's done, we'll be able to tackle all of the prefer-*, as they're the main rules that consume those methods.

I've got a few more rules to convert & cherry pick first tho - I'm catching a flight in a day or so, so I might go dark for a bit, but at this rate I'd say we should easily be done by the end of month, if not by next week :D

SimenB · 2019-07-20T11:08:43Z

Haha, this has been awesome, thank you so much for your help!

SimenB · 2019-07-22T00:35:58Z

TS ESLint had a stable release, so I merged the PR to master. Also updated OP

G-Rath · 2019-07-22T23:22:37Z

I merged the PR to master

Well, this went about as well as I expected - I woke up to about 20 emails for this repo, half of which being semantic bot 😂

but seriously it actually did go as expected: the bugs I've seen are pretty much what I expected, and the fixing PRs have been great, and will make everything more stable.

I'll look to power through getting the rest of the rules converted over the next couple of days.
Pretty much all the rules that are left to do I've got half converted, as I'm picking the ones w/ the least amount of imports from tsUtils first.

SimenB · 2019-07-23T00:52:52Z

Heh, yeah... It wasn't too bad, though!

G-Rath · 2019-07-28T20:35:08Z

@SimenB Just wanted to let you know I've not forgotten about this 😉

I've got 2 rules left to convert, and the new guards + expect parser is working a treat.

Life got a bit busy once I got back, but I hope to have a PR for review by midweek.

As an example of how nice the new expect stuff is: prefer-to-be-null & prefer-to-be-undefined took literally 5 minutes to convert, despite being the two rules that used those massive expect guards, as they were having to account for not.

Now they're just:

const isNullEqualityMatcher = (
  matcher: ParsedExpectMatcher,
): matcher is ParsedNullEqualityMatcher =>
  EqualityMatcher.hasOwnProperty(matcher.name) &&
  matcher.node.parent !== undefined &&
  matcher.node.parent.type === AST_NODE_TYPES.CallExpression &&
  hasOneArgument(matcher.node.parent) &&
  isNullLiteral(matcher.node.parent.arguments[0]);

/* in create: */
if (!isExpectCall(node)) {
  return;
}

const { matcher } = parseExpectCall(node);

if (matcher && isNullEqualityMatcher(matcher)) {
  context.report({
    fix(fixer) {
      return [
        fixer.replaceText(matcher.node.property, 'toBeNull'),
        fixer.remove(matcher.node.parent.arguments[0]),
      ];
    },
    messageId: 'useToBeNull',
    node: matcher.node.property,
  });
}

SimenB · 2019-07-29T08:21:20Z

This is super exciting 😀

G-Rath · 2019-07-29T09:04:57Z

Except I missed the fact that you can chain modifiers (i.e resolve.not) 😭

Not a big deal since we only really check for them in valid-expect, but means I have to code up a linked-list style loop to the parser which are always a bit tedious in TS

SimenB · 2019-07-29T09:14:08Z

You can only have expect().not.toBe(), expect().resolves.toBe(), expect().rejects.toBe(), expect().resolves.not.toBe() and expect().rejects.not.toBe(). So I don't think we necessarily need a linked list? If we can say a parsed expect may or may not be not, and it might be either resolves or rejects. Dunno, might make it hard to do .parent etc?

G-Rath · 2019-07-29T09:32:10Z

Interesting. The typescript types allow it, but it fails at runtime: http://puu.sh/DYIn4/7278eb6827.png

When I say "linked list" it's not anything complex, just:

export const parseExpectCall = (expectCall: ExpectCall): Expectation => {
  const expectation: Expectation = { expectCall };

  if (!isExpectMember(expectCall.parent)) {
    return expectation;
  }

  let node = expectCall.parent;
  let previousModifier: { modifier?: ParsedExpectModifier } = expectation;

  while (node.parent && isExpectMember(node.parent)) {
    const member = parseExpectMember(node.parent);

    if (!isParsedExpectModifier(member)) {
      expectation.matcher = member;
      break;
    }
    previousModifier = previousModifier.modifier = member;

    node = node.parent;
  }

  return expectation;
};

It was more a bit of a shock b/c I'd not considered it, and so it wasn't in the output 😬
Doing it this way means I don't have to refactor anything else, and also has the bonus of requiring the previous modifier to be defined (otherwise parsedExpect.modifer !== undefined && parsedExpect.modifier2 !== undefined)

SimenB · 2019-07-29T11:28:55Z

That should probably be a type error, yeah. The Jest team doesn't maintain those typings, but Jest's internal typings are probably wrong in this regard as well. I'll make a point to fix it before releasing Jest 25

G-Rath · 2019-07-29T23:18:26Z

Ahh I see how it is - I had a proper read over everything, and yeah that loop while nice is overkill (plus the types are murder):

  describe('each', () => {
    it.each([
      expect(true).not.resolves,  // true
      expect(true).not.rejects,    // true
      expect(true).resolves.not, // false
      expect(true).rejects.not    // false
    ])('should be undefined', (result) => {
      expect(result).toBe(undefined);
    });
  });

So the allowed modifiers are:

not
resolves
rejects
resolves.not
rejects.not

However, valid-expect doesn't actually check for that. In fact, it has those in its tests as valid.
In fact, the majority of its not valid tests have not first.

I'll avoid "fixing" that if possible currently, and instead look into it after I've gotten the remaining rules converted & merged.

But in light of this I think the typings can be a lot simpler. I have a few ideas on a couple of different ways it could be typed, and will have a play around to find out what works best :)

G-Rath · 2019-08-01T19:55:25Z

------------------------------|----------|----------|----------|----------|-------------------|
File                          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------------|----------|----------|----------|----------|-------------------|
All files                     |    96.26 |    92.62 |    89.54 |     96.4 |                   |
 src                          |      100 |      100 |      100 |      100 |                   |
  index.js                    |      100 |      100 |      100 |      100 |                   |
 src/processors               |      100 |      100 |      100 |      100 |                   |
  snapshot-processor.js       |      100 |      100 |      100 |      100 |                   |
 src/rules                    |    96.21 |    92.55 |    89.26 |    96.35 |                   |
  consistent-test-it.ts       |      100 |      100 |      100 |      100 |                   |
  expect-expect.ts            |      100 |      100 |      100 |      100 |                   |
  expectGuards.ts             |    72.73 |    47.27 |    53.33 |    72.73 |... 17,424,425,450 |
  final.ts                    |    92.54 |    83.87 |       84 |    92.42 |... 43,365,367,433 |
  lowercase-name.ts           |      100 |      100 |      100 |      100 |                   |
  no-alias-methods.ts         |      100 |      100 |      100 |      100 |                   |
  no-commented-out-tests.ts   |      100 |      100 |      100 |      100 |                   |
  no-disabled-tests.ts        |      100 |      100 |      100 |      100 |                   |
  no-duplicate-hooks.ts       |      100 |      100 |      100 |      100 |                   |
  no-empty-title.ts           |      100 |      100 |      100 |      100 |                   |
  no-export.ts                |      100 |      100 |      100 |      100 |                   |
  no-focused-tests.ts         |      100 |      100 |      100 |      100 |                   |
  no-hooks.ts                 |      100 |      100 |      100 |      100 |                   |
  no-identical-title.ts       |      100 |      100 |      100 |      100 |                   |
  no-if.ts                    |      100 |      100 |      100 |      100 |                   |
  no-jasmine-globals.ts       |      100 |      100 |      100 |      100 |                   |
  no-jest-import.ts           |      100 |      100 |      100 |      100 |                   |
  no-large-snapshots.js       |      100 |      100 |      100 |      100 |                   |
  no-mocks-import.ts          |      100 |      100 |      100 |      100 |                   |
  no-standalone-expect.ts     |      100 |      100 |      100 |      100 |                   |
  no-test-callback.ts         |      100 |      100 |      100 |      100 |                   |
  no-test-prefixes.ts         |      100 |      100 |      100 |      100 |                   |
  no-test-return-statement.ts |      100 |      100 |      100 |      100 |                   |
  no-truthy-falsy.ts          |      100 |      100 |      100 |      100 |                   |
  no-try-expect.ts            |      100 |      100 |      100 |      100 |                   |
  prefer-called-with.ts       |      100 |      100 |      100 |      100 |                   |
  prefer-expect-assertions.ts |      100 |      100 |      100 |      100 |                   |
  prefer-inline-snapshots.ts  |      100 |      100 |      100 |      100 |                   |
  prefer-spy-on.ts            |      100 |      100 |      100 |      100 |                   |
  prefer-strict-equal.ts      |      100 |      100 |      100 |      100 |                   |
  prefer-to-be-null.ts        |      100 |      100 |      100 |      100 |                   |
  prefer-to-be-undefined.ts   |      100 |      100 |      100 |      100 |                   |
  prefer-to-contain.ts        |      100 |      100 |      100 |      100 |                   |
  prefer-to-have-length.ts    |      100 |      100 |      100 |      100 |                   |
  prefer-todo.ts              |      100 |      100 |      100 |      100 |                   |
  require-tothrow-message.ts  |      100 |      100 |      100 |      100 |                   |
  tsUtils.ts                  |    89.86 |    75.38 |    73.08 |    89.71 |... 42,145,166,178 |
  util.js                     |       65 |        0 |    22.22 |    70.59 |     7,10,15,20,29 |
  valid-describe.ts           |      100 |      100 |      100 |      100 |                   |
  valid-expect-in-promise.ts  |      100 |      100 |      100 |      100 |                   |
  valid-expect.ts             |      100 |    98.75 |      100 |      100 |               274 |
------------------------------|----------|----------|----------|----------|-------------------|

Guess who got valid-expect typesafe last night ;)

~~I've just got no-large-snapshots to convert, which will be interesting b/c it uses babel-eslint - hopefully it'll have TS typings, but if not I might have to leave that one to you @SimenB~~

I've now finished no-large-snapshots, resulting in babel-eslint not being needed anymore, as all the tests have been merged into the RuleTester (meaning the .snap is also gone).

I expect to have a PR sometime this weekend 😄

(of course after saying that I realised I should re-review the guards for tests, hooks & describe 😂)

SimenB · 2019-08-16T07:23:21Z

This is done! Thank you to everyone who contributed, and especially @G-Rath who did all the hard work 🎉

SimenB added the help wanted label May 10, 2019

This was referenced May 10, 2019

chore: precompile with babel #257

Merged

chore: port lowercase-name to TypeScript #258

Merged

SimenB added the good first issue label May 12, 2019

ranyitz mentioned this issue May 22, 2019

fix: move @typescript-eslint/experimental-utils to dependencies #267

Merged

Mark1626 mentioned this issue Jul 18, 2019

chore: migrate valid-describe to ts #308

Merged

SimenB mentioned this issue Jul 20, 2019

Assert that async expects are awaited or returned - Closes #54, #254 #255

Merged

4 tasks

SimenB mentioned this issue Jul 21, 2019

prefer-to-be-null TypeError with TSAsExpression #332

Closed

G-Rath mentioned this issue Aug 4, 2019

ts-migration/everything-else #363

Closed

SimenB mentioned this issue Aug 12, 2019

Support ESLint's --fix-type option #202

Closed

SimenB closed this as completed Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite in TypeScript #256

Rewrite in TypeScript #256

SimenB commented May 10, 2019 •

edited by G-Rath

G-Rath commented May 26, 2019 •

edited

jeysal commented May 27, 2019

SimenB commented May 28, 2019

G-Rath commented May 29, 2019 •

edited

SimenB commented Jun 3, 2019

Mark1626 commented Jun 23, 2019

SimenB commented Jul 17, 2019 •

edited

SimenB commented Jul 18, 2019

G-Rath commented Jul 18, 2019

SimenB commented Jul 18, 2019

G-Rath commented Jul 18, 2019

G-Rath commented Jul 19, 2019 •

edited

SimenB commented Jul 19, 2019 •

edited

G-Rath commented Jul 19, 2019

SimenB commented Jul 19, 2019

G-Rath commented Jul 20, 2019

SimenB commented Jul 20, 2019

SimenB commented Jul 22, 2019 •

edited

G-Rath commented Jul 22, 2019

SimenB commented Jul 23, 2019

G-Rath commented Jul 28, 2019 •

edited by SimenB

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019 •

edited by SimenB

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019 •

edited

G-Rath commented Aug 1, 2019 •

edited

SimenB commented Aug 16, 2019

Rewrite in TypeScript #256

Rewrite in TypeScript #256

Comments

SimenB commented May 10, 2019 • edited by G-Rath

G-Rath commented May 26, 2019 • edited

jeysal commented May 27, 2019

SimenB commented May 28, 2019

G-Rath commented May 29, 2019 • edited

SimenB commented Jun 3, 2019

Mark1626 commented Jun 23, 2019

SimenB commented Jul 17, 2019 • edited

SimenB commented Jul 18, 2019

G-Rath commented Jul 18, 2019

SimenB commented Jul 18, 2019

G-Rath commented Jul 18, 2019

G-Rath commented Jul 19, 2019 • edited

SimenB commented Jul 19, 2019 • edited

G-Rath commented Jul 19, 2019

SimenB commented Jul 19, 2019

G-Rath commented Jul 20, 2019

SimenB commented Jul 20, 2019

SimenB commented Jul 22, 2019 • edited

G-Rath commented Jul 22, 2019

SimenB commented Jul 23, 2019

G-Rath commented Jul 28, 2019 • edited by SimenB

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019 • edited by SimenB

SimenB commented Jul 29, 2019

G-Rath commented Jul 29, 2019 • edited

G-Rath commented Aug 1, 2019 • edited

SimenB commented Aug 16, 2019

SimenB commented May 10, 2019 •

edited by G-Rath

G-Rath commented May 26, 2019 •

edited

G-Rath commented May 29, 2019 •

edited

SimenB commented Jul 17, 2019 •

edited

G-Rath commented Jul 19, 2019 •

edited

SimenB commented Jul 19, 2019 •

edited

SimenB commented Jul 22, 2019 •

edited

G-Rath commented Jul 28, 2019 •

edited by SimenB

G-Rath commented Jul 29, 2019 •

edited by SimenB

G-Rath commented Jul 29, 2019 •

edited

G-Rath commented Aug 1, 2019 •

edited