ts-migration/everything-else #363

G-Rath · 2019-08-04T12:54:37Z

It's finally here!

This PR converts the remaining JS files to TypeScript (where possible) - All tests are passing, and coverage is 100%; as such this PR is good to go as is.

For now I'm going to do some light cleanup, which I'll commit & push to this branch, but can also be in the next PR - let me know if you'd like me to cherry pick anything out into separate PRs & what have you.

I am trying to avoid changing the already-converted rules in this PR, which is why there is definitely some redundancy, and forms a nice bridge to the second half of what is quickly becoming one of my classic overly detailed text dumps:

My immediate next step is going over every rule w/ a fine-tooth comb to make sure they're all consistent, and to fix a number of nitpicks, including:

property order (i.e valid above invalid in rule tests)
using the new parseExpectCall where possible, & other such new methods
ensure the same property name is used in data (i.e name vs propertyName vs matcherName)
checking all the meta for each rule is correct (recommended, description, etc)
merging redundant types & code
use template strings instead of .join('\n') for multi-line strings

The big massive inconsistency in the room that I'd like to get addressed is our stance on bracket accessors:

expect(files['length']).toBe(1);
expect(/* args */)['not'].toBe(/* args */);
describe['skip'](/* args */);

A few tests have explicit test cases showing support (i.e describe['skip']), but most other rules either won't work w/ that type of accessor, or - in the case of fixers - will error.

The former is not a problem, as the AccessorNode & co (getAccessorValue, isSpecificAccessor, etc) types & methods are for facilitating the normalisation required for rules to support both Identifier (expect.<blah>) & literal node types (expect['blah']).

However, all the fixers currently will just error out - this is b/c the ones that this matter for are typically searching for a propertyDot value.

It's fairly straightforward to implement support, and am happy to do it - but it's something that other maintainers should be aware of for when reviewing new rules, as we should try and ensure all rules have tests for both types of accessor.

In light of that, I'm going look to create a series of standard tests that all rules should have to be tested against, w/ things like expect();, expect, expect.toBe(), etc to make sure that all our rules can handle the same basic edge cases.

I also want to review the stuff for test, describe, etc, & getNodeName as they're somewhat duck-taped together. Ideally it'd be cool if I could create a "parse" method for those too, but that might be a bit overkill - either way, there are some open issues relating to them so let's see what I can come up w/ :D

Following all of this, which shouldn't take too long, I'm going to look at open issues, and try to get some of them closed, including implementing some new rules :)

In particular, I want to create a "ban-matchers" rule as it's just a form of no-truthy-falsy w/ an array option, but I can't nail down a good syntax for supporting modifiers which is making me super sad :( - I'll probably open up a new issue for this to discuss, while making a basic version of the rule to start w/.

Fixes #256
Relates to #332
Superseeds & closes #346
Superseeds & closes #333

G-Rath · 2019-08-04T12:56:43Z

@SimenB I was planning on doing some minor clean up before bed, but by the time I hit the "create" button it was 1am 😂

Feel free to review & request changes, but as I said in the PR body I think at this point it's probably best to merge first & cleanup after, just b/c of the share size of the PR.

oh and btw I migrated the dangerfile to TS, but no idea if it's working, as I've not used danger before 😬

Would you be able to check that out for me?

babel requires #8529 to be converted to TS, & I couldn't find anything for eslint supporting .ts config :'(

I created an issue for eslint support .eslintrc.ts

DangerCI · 2019-08-04T13:14:36Z

	Fails
🚫	Danger failed to run `dangerfile.ts`.

Error SyntaxError

Unexpected token var
dangerfile.ts:5
declare var danger: {
        ^^^

SyntaxError: Unexpected token var
    at Module._compile (internal/modules/cjs/loader.js:723:23)
    at requireFromString (/home/travis/.npm/_npx/5531/lib/node_modules/danger/node_modules/require-from-string/index.js:28:4)
    at Object.<anonymous> (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:134:34)
    at step (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:32:23)
    at Object.next (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:13:53)
    at /home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:7:71
    at new Promise (<anonymous>)
    at __awaiter (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:3:12)
    at Object.exports.runDangerfileEnvironment (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/runner/runners/inline.js:95:136)
    at Object.<anonymous> (/home/travis/.npm/_npx/5531/lib/node_modules/danger/distribution/commands/danger-runner.js:83:55)

Dangerfile

--------------------------^

Generated by 🚫 dangerJS

G-Rath · 2019-08-04T20:11:17Z

@SimenB errrrr I'm going to defer this to you rather than jump to straight to reporting it to yarn, since it only happened on the node@8 build :/

206319.65s$ curl -o- -L https://yarnpkg.com/install.sh | bash
207Installing Yarn!
208/usr/local/bin/yarn
209> 1.15.2 is already installed, Specified version: 1.17.3.
210> Downloading tarball...
211
212[1/2]: https://yarnpkg.com/latest.tar.gz --> /tmp/yarn.tar.gz.38irGXyMeE
213  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
214                                 Dload  Upload   Total   Spent    Left  Speed
215100    93  100    93    0     0    443      0 --:--:-- --:--:-- --:--:--   444
216100   609    0   609    0     0   1108      0 --:--:-- --:--:-- --:--:--  4544
217  2 1211k    2 34378    0     0    108      0  3:11:26  0:05:17  3:06:09     0
218curl: (56) GnuTLS recv error (-54): Error in the pull function.
219
220[2/2]: https://yarnpkg.com/latest.tar.gz.asc --> /tmp/yarn.tar.gz.38irGXyMeE.asc
221100    97  100    97    0     0    790      0 --:--:-- --:--:-- --:--:--   790
222100   613    0   613    0     0   1532      0 --:--:-- --:--:-- --:--:--  1532
223100   832  100   832    0     0   1306      0 --:--:-- --:--:-- --:--:--  1306
224> Verifying integrity...
225gpg: keyring `/home/travis/.gnupg/secring.gpg' created
226gpg: key 86E50310: public key "Yarn Packaging <yarn@dan.cx>" imported
227gpg: Total number processed: 1
228gpg:               imported: 1  (RSA: 1)
229gpg: Signature made Fri 12 Jul 2019 02:31:25 PM UTC using RSA key ID 69475BAA
230gpg: BAD signature from "Yarn Packaging <yarn@dan.cx>"
231> GPG signature for this Yarn release is invalid! This is BAD and may mean the release has been tampered with. It is strongly recommended that you report this to the Yarn developers.
232The command "curl -o- -L https://yarnpkg.com/install.sh | bash" failed and exited with 1 during .

SimenB · 2019-08-04T20:18:50Z

Just retrigger the build - you should have the correct rights as you have push access to the repo. The failure looks transient.

I'll review this tomorrow or Wednesday (I'm currently on vacation, but in transit tomorrow, so might have some time then)

G-Rath · 2019-08-04T20:21:33Z

Yeah that's what I figured.

Sorry about the sudden email spam btw 😬 Feel free to take your time on responding - I've got more than enough things on my plate that I'm in no rush 😂

SimenB

This is super exciting! I'm not done reviewing, I just did a quick pass 🙂 Will come back to it later, but might as well submit in the meantime

dangerfile.ts

package.json

src/globals.json

src/processors/snapshot-processor.ts

src/rules/__tests__/prefer-to-contain.test.ts

src/rules/no-disabled-tests.ts

src/rules/no-large-snapshots.ts

SimenB · 2019-08-06T07:19:17Z

src/rules/prefer-called-with.ts

+
+        const { modifier, matcher } = parseExpectCall(node);
+
+        // Could check resolves/rejects here but not a likely idiom.


if anything it should be an error...

src/rules/prefer-expect-assertions.ts

G-Rath · 2019-08-06T08:03:35Z

Awesome, thanks for the first-pass review!

I figured you'd like some of that stuff broken out into PRs, but at the time just wanted to get it all committed 😂

I'll start cherry-picking.

I've also got an idea on how to implement a parseTestCase version of paseExpectCall :D

SimenB

This is amazing work, thank you so much for tackling this. This has many hours invested in it, and I highly appreciate it! ❤️

My only real comment is that I think the utils are hard to navigate, even though they seem well documented. While we could deal with that in a follow-up, I'd prefer it if we could figure something out with them before landing this

src/index.ts

SimenB · 2019-08-06T09:21:16Z

src/rules/no-large-snapshots.ts

-            propertyName === 'toThrowErrorMatchingInlineSnapshot'
+            'property' in node.callee &&
+            isSupportedAccessor(node.callee.property) &&
+            [


extract this array into a const outside of the scope of this function?

SimenB · 2019-08-06T09:37:41Z

src/rules/utils.ts

+ *
+ * @return {node is StringLiteral}
+ */
+export const isStringLiteral = (node: TSESTree.Node): node is StringLiteral =>


don't export this, it's not used. Mind going over all exports and just keeping the ones actually used outside of this file as exports?

Yup that's at the top of my list of things to do during the week - same w/ checking all the generics :)

SimenB · 2019-08-06T09:39:51Z

src/rules/utils.ts

+  | NotNegatableParsedModifier<NotNegatableModifierName>
+  | NegatableParsedModifier<NegatableModifierName>;
+
+interface Expectation<ExpectNode extends ExpectCall = ValidExpectCall> {


Should we have some sort of expect-utils file? I think this file is big and unwieldy so I'd like to split it up somehow. Navigating the helpers might be difficult if you're not familiar with them already.

If that's hard, could we group types and their type guards separately from the parsers etc?

That's exactly what I'm thinking, and planning to look into.
Glad we're on the same page :)

src/rules/utils.ts

SimenB · 2019-08-06T09:42:06Z

src/rules/utils.ts

+
+  /* istanbul ignore next */
+  if (
+    !(


way too many parens

if ( !isSpecificMember(parsedMember, ModifierName.resolves) && !isSpecificMember(parsedMember, ModifierName.rejects) ) { throw }

Maybe even isSpecificMember should take varargs at the end so you can do isSpecificMember(parsedMember, ModifierName.resolves, ModifierName.rejects)?

Agreed - I'm not sure I went w/ that in the first place 😂

While varargs would work, I didn't use them b/c I think it's not worth it - this is the only place that method is used, and w/ using varargs you've got to & string[] for .includes to work properly; I don't think the mess is worth the gain here.

Overall the whole reason we need this check is that TypeScript knows that while compile time we enforce name & getAccessorValue(node) to be the "same" (since name is meant to be literally the result of getAccessorValue(node)), at runtime there is no such enforcement.

It's a bit of a messy situation, that I think is partly due to how I've structured the typings to account for negation, but imo it's an acceptable trade-off: this is as unsound as we get relative to the other types, and they should be as stable as they can get.

The alternative types would likely be very confusing & drawn out, for the same result minus having to do this check at runtime.

I think this will be solvable (or at least improvable) once negated types land, of which this library is one I've got earmarked as a nice place to play around w/ them in b/c there's some clear places they could be used, so hopefully they'll land soon.

G-Rath · 2019-08-06T09:53:07Z

Thanks again for the review!

Looking over it I think the majority of your comments are what I had when I self-reviewed, and are on my radar as part of the clean up, which is awesome :D

I'll tackle it during the week - should have it all cleaned by the weekend at the latest :)

SimenB · 2019-08-06T09:57:31Z

That's awesome, looking forward to it!

G-Rath · 2019-08-06T10:06:14Z

Thanks, me too. In the meantime if you're feeling daring, might be worth releasing the stuff we just merged (after the snapshot processor & index are merged) just so that any (hopefully small) breaks trickle through now to be tackled.

My work schedule is such that I can handle triaging & tackling any smaller issues that pop up, but will be limited on core hours until Friday (hence why the major clean will probs be mainly on the weekend).

Up to you :)

SimenB · 2019-08-06T20:17:54Z

src/rules/__tests__/valid-expect.test.ts

  invalid: [
+    /*


failing tests? If yes, open up an issue so we don't forget?

G-Rath · 2019-08-06T20:46:13Z

@SimenB your rebase had the same results as mine.

Either we'll have to interactively rebase to resolve it manually, or just make a new commit. I'm happy if you've got the time to do the former, otherwise I can just do the latter & call it a day :)

SimenB · 2019-08-06T22:12:11Z

Just a new commit is fine. I'll probably rewrite a bunch of the cleanup commits either way before merging 🙂

G-Rath · 2019-08-06T22:46:25Z

Just a new commit is fine.

Awesome, I'll do that now then.

I'll probably rewrite a bunch of the cleanup commits either way

I'm open to being schooled if there's anything I could improve on to help reduce the need for rewriting on future work :)

SimenB · 2019-08-06T23:14:53Z

Not much to do until we're ready to merge, really 🙂 As the review is ongoing, new commits all the time are best as it's easiest to review that way. But stuff like "add comment", "rename function" and "change ternary order" (or something like that) is not useful when merged to master.

dangerfile.ts

SimenB · 2019-08-07T16:19:37Z

heads up - I merged #364. Which I guess will be even simpler with the parseExpect from this PR 🙂

(that will also trigger the release you requested with some of the refactors extracted from this PR)

SimenB

This PR is so big 😅 Do you think it'd be possible to land a few of the new guards on master outside of a 2k LOC diff?

Also, maybe we could keep tsUtils named utils in this PR, which should remove a bunch of the files from the diff, and just have a single commit later renaming the file and fixing the imports

src/rules/tsUtils.ts

src/rules/prefer-todo.ts

src/rules/no-large-snapshots.ts

src/rules/no-identical-title.ts

G-Rath · 2019-08-10T21:50:13Z

@SimenB haha yeah that's what I'm thinking too.

Right now I'm actually going through all the CLI options in Jest to find out which ones don't yet support being transformed ;)

But yeah I think that's best - I'll whip up some PRs.

Do you think it'd be possible to land a few of the new guards on master outside of a 2k LOC diff?

Are you saying land them w/o rules? I'm happy w/ that if you are :)

SimenB · 2019-08-10T21:52:53Z

I meant the guards that are used in the already converted rules. That way it might be easier to properly review parseExpect and friends :) If a guard is just used in a freshly converted rule, I think they should stay together as they're often easier to review when they're also used

src/rules/no-disabled-tests.ts

SimenB · 2019-08-11T12:59:32Z

src/rules/no-identical-title.ts

@@ -1,3 +1,4 @@
+import { TSESTree } from '@typescript-eslint/experimental-utils';


is this import used? other changes in file seems to be just whitespace now that it landed on master, so would be cool to avoid that diff?

Yeah that's weird - I'll check this out.
Those whitespace changes were in my PR to master iirc, so not sure what's happened.

Feel free to drop changes to this file when rebasing :)

src/rules/prefer-expect-assertions.ts

src/rules/valid-expect-in-promise.ts

G-Rath · 2019-08-11T19:56:39Z

src/rules/no-identical-title.ts

@@ -1,3 +1,4 @@
+import { TSESTree } from '@typescript-eslint/experimental-utils';


Yeah that's weird - I'll check this out.
Those whitespace changes were in my PR to master iirc, so not sure what's happened.

Feel free to drop changes to this file when rebasing :)

src/rules/no-disabled-tests.ts

G-Rath · 2019-08-11T19:58:08Z

src/rules/no-alias-methods.ts

@@ -1,19 +1,25 @@
-import { expectCaseWithParent, getDocsUrl, method } from './util';
+import { createRule, isExpectCall, parseExpectCall } from './tsUtils';


I'll probably use this to land parseExpectCall, followed by no-truthy-falsy, and then the other rules can have their own PRs 🤷‍♂

SimenB · 2019-08-11T22:47:28Z

I don't think the commits here are very atomic any more (most of the work is in convert everything else to typescript, and most of the others are renaming and refactoring code introduced in that commit). I've been rebasing locally for some time and concluded with just squashing everything down to a single commit :P Did reduce the diff vs master, though.

(If I messed up, reflog can rollback)

G-Rath · 2019-08-11T22:51:00Z

That's ok - I don't think we'll be landing this PR specifically, but it's good to have for tracking work remaining.

When I get a chance I'll break a few more rules out for merging :)

Also: only ~1k LOC changes now :D

SimenB · 2019-08-11T22:52:17Z

Makes sense! I think it was a good idea with a huge PR though, to get a feeling of the final picture, but splitting parts out when things start settling into pieces that are actually reviewable makes sense 🙂

G-Rath · 2019-08-16T04:50:18Z

@SimenB it is done 🚀🚀🚀🚀🚀🚀🚀🚀

G-Rath requested a review from SimenB August 4, 2019 12:54

G-Rath force-pushed the ts-migration/everything-else branch 2 times, most recently from 4358e44 to 9c5bb95 Compare August 4, 2019 13:09

SimenB closed this Aug 4, 2019

SimenB reopened this Aug 4, 2019

G-Rath mentioned this pull request Aug 5, 2019

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

Closed

SimenB reviewed Aug 6, 2019

View reviewed changes

G-Rath mentioned this pull request Aug 6, 2019

[prefer-to-contain] handle bracket accessor properties #369

Closed

G-Rath force-pushed the ts-migration/everything-else branch from defc039 to 8097c7b Compare August 6, 2019 09:04

SimenB requested changes Aug 6, 2019

View reviewed changes

G-Rath mentioned this pull request Aug 6, 2019

ts-migration/index #372

Merged

G-Rath force-pushed the ts-migration/everything-else branch from 405b2e8 to d896302 Compare August 6, 2019 10:35

SimenB force-pushed the ts-migration/everything-else branch from 044db14 to 7bc69ac Compare August 6, 2019 20:13

SimenB reviewed Aug 6, 2019

View reviewed changes

src/rules/__tests__/valid-expect.test.ts Outdated

invalid: [

/*

Copy link

Member

SimenB Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failing tests? If yes, open up an issue so we don't forget?

SimenB reviewed Aug 7, 2019

View reviewed changes

dangerfile.ts Outdated Show resolved Hide resolved

G-Rath force-pushed the ts-migration/everything-else branch from ca9b704 to bbe5238 Compare August 8, 2019 10:58

G-Rath force-pushed the ts-migration/everything-else branch 2 times, most recently from fdb983e to a46abf9 Compare August 10, 2019 05:51

SimenB reviewed Aug 10, 2019

View reviewed changes

src/rules/tsUtils.ts Outdated Show resolved Hide resolved

src/rules/prefer-todo.ts Outdated Show resolved Hide resolved

src/rules/no-large-snapshots.ts Outdated Show resolved Hide resolved

src/rules/no-identical-title.ts Outdated Show resolved Hide resolved

G-Rath force-pushed the ts-migration/everything-else branch 4 times, most recently from 8393435 to 9f3c919 Compare August 11, 2019 12:30

SimenB reviewed Aug 11, 2019

View reviewed changes

src/rules/no-disabled-tests.ts Outdated Show resolved Hide resolved

SimenB reviewed Aug 11, 2019

View reviewed changes

SimenB mentioned this pull request Aug 11, 2019

ts-migration/prefer-called-with #346

Closed

G-Rath force-pushed the ts-migration/everything-else branch from 9f3c919 to 24f1858 Compare August 11, 2019 13:28

G-Rath mentioned this pull request Aug 11, 2019

ts-migration/no-large-snapshots #376

Merged

G-Rath commented Aug 11, 2019

View reviewed changes

SimenB force-pushed the ts-migration/everything-else branch from 24f1858 to 904996d Compare August 11, 2019 22:50

G-Rath force-pushed the ts-migration/everything-else branch 5 times, most recently from 41d6088 to c6fb4da Compare August 16, 2019 04:45

G-Rath closed this Aug 16, 2019

G-Rath force-pushed the ts-migration/everything-else branch from c6fb4da to 26756b6 Compare August 16, 2019 04:49

G-Rath deleted the ts-migration/everything-else branch August 16, 2019 04:50


		const { modifier, matcher } = parseExpectCall(node);

		// Could check resolves/rejects here but not a likely idiom.

		@@ -1,3 +1,4 @@
		import { TSESTree } from '@typescript-eslint/experimental-utils';

		@@ -1,19 +1,25 @@
		import { expectCaseWithParent, getDocsUrl, method } from './util';
		import { createRule, isExpectCall, parseExpectCall } from './tsUtils';

ts-migration/everything-else #363

ts-migration/everything-else #363

Conversation

G-Rath commented Aug 4, 2019 • edited

G-Rath commented Aug 4, 2019 • edited

DangerCI commented Aug 4, 2019 • edited

Error SyntaxError

Dangerfile

G-Rath commented Aug 4, 2019

SimenB commented Aug 4, 2019 • edited

G-Rath commented Aug 4, 2019

SimenB left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath commented Aug 6, 2019

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath commented Aug 6, 2019 • edited

SimenB commented Aug 6, 2019

G-Rath commented Aug 6, 2019

Choose a reason for hiding this comment

G-Rath commented Aug 6, 2019

SimenB commented Aug 6, 2019

G-Rath commented Aug 6, 2019

SimenB commented Aug 6, 2019

SimenB commented Aug 7, 2019 • edited

SimenB left a comment

Choose a reason for hiding this comment

G-Rath commented Aug 10, 2019

SimenB commented Aug 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Aug 11, 2019

G-Rath commented Aug 11, 2019 • edited

SimenB commented Aug 11, 2019

G-Rath commented Aug 16, 2019

G-Rath commented Aug 4, 2019 •

edited

G-Rath commented Aug 4, 2019 •

edited

DangerCI commented Aug 4, 2019 •

edited

SimenB commented Aug 4, 2019 •

edited

SimenB left a comment •

edited

G-Rath commented Aug 6, 2019 •

edited

SimenB commented Aug 7, 2019 •

edited

G-Rath commented Aug 11, 2019 •

edited