Typescript conversion #32

rossknudsen · 2020-01-25T20:56:54Z

This is a conversion to Typescript, removing Flow and updating build scripts.

Unit tests should be passing, but I need to manually test. Maybe we can get a pre-release, or is there another simple way to substitute an npm package in my extension?

Added types for babylon

Converted more to Typescript Some more conversions

rossknudsen · 2020-01-25T21:38:27Z

Hmmm, looks like I need to fix the linting...

rossknudsen · 2020-01-26T09:47:47Z

Ah looks like the version of node on the CI server is too low to run one of the new dependencies. Can we bump the version?

connectdotz · 2020-01-26T16:58:40Z

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

stephtr · 2020-01-26T21:27:59Z

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

Personally, I definitely prefer working with TypeScript. It would also better fit to vscode-jest.

stephtr · 2020-01-26T21:29:03Z

.eslintrc.js

-  extends: ['airbnb-base', 'plugin:flowtype/recommended', 'prettier', 'plugin:prettier/recommended'],
+  extends: [
+    'airbnb-typescript/base',
+    "airbnb/hooks",


I guess we should use quotes consistently 😉

My choice would have been for double quotes, but I tried to stick with the Prettier rules that were already present.

I was just refering to some extends strings being single- and some being doublequoted. I would prefer enforcing consistent quotes for the whole project.

Ah my bad. Was reading that comment on mobile. Not sure why I've got a mixture there. Will fix.

stephtr · 2020-01-26T21:30:37Z

fixtures/dangerjs/github.example

@@ -1,4 +1,4 @@
-// @flow
+


Why do some files start with an empty line?

Thats a good question. It took me ages to figure out why some unit tests were failing. Turns out the line numbers are hard-coded in them and I was trying to change the minimal amount of code, so that this PR was readable.

So I didn't update the tests with new line numbers.

stephtr · 2020-01-26T21:31:30Z

fixtures/parser_tests.ts

 const fixtures = __dirname;

-function parserTests(parse: (file: string) => ParserResult) {
+function parserTests(parse: (file: string, data?: string | undefined) => ParseResult) {


The ? already contains undefined.

stephtr

I didn't have time to go through all changes, but these are my comments for some of them.

stephtr · 2020-01-26T21:32:22Z

fixtures/parser_tests.ts

+    name: string | null = null,
  ) => assertBlock(block, {column: sc, line: sl}, {column: ec, line: el}, name);
-  const assertBlock = (block, start, end, name: ?string = null) => {
+  const assertBlock = (block, start, end, name: string | null = null) => {


We could change this to just name?: string.

stephtr · 2020-01-26T21:33:43Z

fixtures/parser_tests.ts


      // check test blocks, including the template literal
-      const found = descBlock.children.filter(
+      const found = descBlock?.children?.map(b => b as ItBlock)?.filter(


In my opinion we don't need a ?. after map.

👍

How did you add these @rossknudsen?

@seanpoulter if you mean this specific piece of code, it now looks like this:

// check test blocks, including the template literal const found = descBlock.children .map(b => b as NamedBlock) .filter( b => b.name === 'needs to have a PR number' || b.name === 'does not validate without josh' || b.name === 'does not validate when ${key} is missing' ); expect(found.length).toBe(3);

I was just trying to assert that the type of the array elements were NamedBlocks (originally ItBlocks). There is potentially another way to write this which is arguably more elegant. Now that the children property is not potentially null, it is a lot clearer.

If you are talking more generally, you might need to give me some more context.

stephtr · 2020-01-26T21:35:04Z

fixtures/parser_tests.ts

+export {
  parserTests,
-};
+}


Why not use export function parserTests?

stephtr · 2020-01-26T21:36:18Z

fixtures/parser_tests.ts

@@ -184,24 +186,24 @@ function parserTests(parse: (file: string) => ParserResult) {
    it('finds test blocks within describe blocks', () => {
      const data = parse(`${fixtures}/dangerjs/travis-ci.example`);
      const descBlock = data.describeBlocks[1];
-      expect(descBlock.children.length).toBe(4);
+      expect(descBlock.children?.length).toBe(4);


Can children really be not an (empty) array? I'm just wondering about the change between old and new code.

stephtr · 2020-01-26T21:38:13Z

src/Runner.ts

 import ProjectWorkspace from './project_workspace';
 import {createProcess} from './Process';

 // This class represents the running process, and
 // passes out events when it understands what data is being
 // pass sent out of the process
 export default class Runner extends EventEmitter {
-  debugprocess: ChildProcess;
+  debugprocess: ChildProcess | null = null;


What about debugprocess?: ChildProcess;?

stephtr · 2020-01-26T21:48:17Z

src/__tests__/process.test.ts

@@ -30,24 +29,24 @@ describe('createProcess', () => {
    const args = [];
    createProcess(workspace, args);

-    expect(spawn.mock.calls[0][0]).toBe('jest');
-    expect(spawn.mock.calls[0][1]).toEqual([]);
+    expect((spawn as any).mock.calls[0][0]).toBe('jest');


One could also use the as Mock<any> type from Jest.

Cool, slightly more type-safe. Any ideas on replacing the any part of the type with something more specific?

stephtr · 2020-01-26T21:50:32Z

src/parsers/parser_nodes.ts


 export class ParsedNode {
  type: ParsedNodeType;

-  start: Location;
+  start: Location | undefined = undefined;


On such occasions start?: Location; would be sufficient.

stephtr · 2020-01-26T21:51:29Z

src/parsers/parser_nodes.ts

@@ -1,14 +1,15 @@
+/* eslint-disable @typescript-eslint/no-use-before-define */
+/* eslint-disable max-classes-per-file */


In my opinion we could also get rid of the max-classes-per-file policy project-wide.

Sure.

What is your opinion of placement of the copyright notice vs linting configuration? I had just left it where the intellisense put it, at the very top.

stephtr · 2020-01-26T21:53:32Z

src/parsers/parser_nodes.ts

+export const ParsedNodeTypes: {
+  describe: 'describe';
+  expect: 'expect';
+  it: 'it';
+  root: 'root';
+} = {
  describe: 'describe',
  expect: 'expect',
  it: 'it',
  root: 'root',
 };


It has to be tested, but this could be replaced by either an enum or export type ParsedNodeTypes = 'describe' | 'expect' | ....

Agreed. I was tempted to rewrite some of the parser code relying on TS more, but I see that #30 is already rewriting the parser code. It would probably make more sense to merge those changes first.

Actually having read that PR, I'm inclined to wait until it is merged because it will remove the typescript parser. That reduces the delta in this PR.

stephtr · 2020-01-26T21:54:04Z

src/parsers/parser_nodes.ts


  file: string;

-  children: ?Array<ParsedNode>;
+  children?: Array<ParsedNode>;


ParsedNode[] is easier to read than Array<...>.

I agree. Pretty sure this is a Prettier/ESLint rule.

Fixed bug introduced in previous commit that broke tests.

rossknudsen · 2020-01-26T22:42:07Z

Huh, somehow the coverage dropped...

rossknudsen · 2020-01-27T03:01:02Z

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

Fair enough, I was going by this comment which is probably old/stale now. I ran across the comment because I wanted to access more of the Jest settings. I guess I went down a bit of a rabbit hole with this one...

seanpoulter · 2020-01-27T04:14:52Z

Thanks for the huge effort @rossknudsen! 👏

--

@rossknudsen wrote

Unit tests should be passing, but I need to manually test. Maybe we can get a pre-release, or is there another simple way to substitute an npm package in my extension?

Yes, you can substitute npm packages using "linking". Here's the docs for npm and yarn that are probably more clear than what I'd write. ;)

--

@connectdotz wrote:

while I like typescript a lot personally, I am not the only contributor in this repo, @stephtr @seanpoulter, are you guys ok to move the repo to typescript?

I've got two answers for this: one with best interest of the project in mind, and the other from my own perspective.

With the best interest in the project in mind, I'd say yes, switching to TypeScript makes sense because folks are more familiar with TypeScript than Flow. I don't think any of the other reasons I've considered sway me one way or the other.

Pros	More type information will help folks use the library in lieu of documentation We can leverage more of the types exported from the facebook/jest repo TypeScript has more adoption than Flow [1] [2] TypeScript may have better IDE integration than Flow TypeScript might improve our code quality
Cons	TypeScript is still a barrier to entry over "regular" "JavaScript" -- not everyone will keep up with the ES20xx language features Spending time on test case coverage and code review may be a better time investment [3]

With my own interest in mind, I see this is a PR that's too big for me to approach so I'm biased towards keeping the status quo. While I think this is adding a lot of value, we may have missed some of the conversations we need like:
- Do the maintainers have the capacity to review the PR? (I don't)
- Can we make the switch from Flow to TypeScript in incremental, easy to review PRs?
- Do we need to discussion what TS config we'll use, and is it familiar? (As an arbitrary example, this is the first time I've seen optional chaining in a repo and sometimes they feel icky, like in tests).
- What test cases do we need to have confidence in this, especially if it's too big to review?
- (edited to add this): Are we OK to migrate AND refactor at the same time? For example, can we convert the Flow Maybe to the TypeScript optional?

[1]: https://2019.stateofjs.com/javascript-flavors/typescript/
[2]: https://2019.stateofjs.com/javascript-flavors/other-tools/
[3]: https://medium.com/javascript-scene/the-typescript-tax-132ff4cb175b

.eslintrc.js

Co-Authored-By: Sean Poulter <[email protected]>

seanpoulter · 2020-01-27T05:18:10Z

src/test_reconciler.ts

-      let line = null;
+      let short: string | undefined;
+      let terse: string | undefined;
+      let line: number | undefined;


The change from null to undefined stands out to me as a potentially risky API change. The risk is that a caller uses all three types from the Flow Maybe type (type | null | void (undefined)) to differentiate between values that aren't in the map/object, are invalid, or are a valid number. It makes me worry that I'd/we'd have to do a more thorough code review or testing.

In this particular case, the caller seems fine but there's still the rough edge of how we're using this.lineOfError(...) || undefined to do ... something. What would ever be falsy enough to end up as undefined?! I actually had to run this in the Dev Tools console just to check the falsy precedence:

I guess I learned (or relearned) that. ☺️ Since I'll probably forget that the next time I look at it, I'd probably suggest going for:

- line = this.lineOfError(message, filename) || undefined; + line = this.lineOfError(message, filename);

with lineOfError returning undefined. I'm not a fan of the magic numbers like [1] in there so I'd hope to see something like:

- lineOfError(message: string, filePath: string): number | null { - const filename = path.basename(filePath); - const restOfTrace = message.split(filename, 2)[1]; - return restOfTrace ? parseInt(restOfTrace.split(':')[1], 10) : null; - } + lineOfError(message: string, filepath: string): ?number { + const filename = path.basename(filepath); + const [_msgIncludingFilename, msgAfterFilename] = message.split(filename, 2); + if (! msgAfterFilename) return; + const [_colon, lineNumber] = msgAfterFilename(':'); + return parseInt(lineNumber, 10); + }

--

The underlying question for the folks reviewing is are we OK with migrating and refactoring at the same time. Here's the docs for the Flow Maybe type.

rossknudsen · 2020-01-27T09:36:14Z

@seanpoulter I appreciate the time taken to write your thoughts regarding this PR. I reviewed the changed files again and it is substantial. My original intent was to avoid a massive delta as it has potential to introduce regressions etc and part of the reason why people wouldn't want to convert in the first place. Also 100% appreciate your limited time in reviewing this.

In light of all this, I stepped back and thought about whether its possible to take an incremental approach to conversion. So I looked at the recommendations for conversion from Microsoft and cherry-picked some changes from this branch into a new branch. If you like this approach I can open another PR for review. I think I've kept everything operational while converting just one file to Typescript.

connectdotz · 2020-01-27T15:37:46Z

In light of all this, I stepped back and thought about whether its possible to take an incremental approach to conversion... If you like this approach I can open another PR for review. I think I've kept everything operational while converting just one file to Typescript.

I like that! 👍

connectdotz · 2020-02-08T17:12:10Z

superseded by #33

rossknudsen added 3 commits January 25, 2020 23:15

Added Typescript, removed Flow. Updated config files.

b22179e

Added types for babylon

Converted main files to Typescript

1ed56ad

Converted more to Typescript Some more conversions

Converted tests to Typescript

9f218cc

rossknudsen requested review from stephtr, connectdotz and orta January 25, 2020 20:57

connectdotz mentioned this pull request Jan 26, 2020

parse does not find name for test.each block #29

Closed

stephtr reviewed Jan 26, 2020

View reviewed changes

rossknudsen added 5 commits January 27, 2020 11:37

Fixed type errors

82c34c2

Fixed bug introduced in previous commit that broke tests.

Updated build configuration.

9d2cbf3

Fixed linting.

07facbc

Try bumping node version to build package.

dcdea4c

Upgraded babel parser and fixed babylon_parser.

2ac03d3

rossknudsen force-pushed the typescript branch from 0e61a26 to 2ac03d3 Compare January 26, 2020 22:37

rossknudsen added 3 commits January 27, 2020 16:22

Modified linting rules for array types and max classes per file.

61f8732

Consistent single quotes.

d24e583

Fixes as per PR suggestions.

7d36e0d

seanpoulter reviewed Jan 27, 2020

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

rossknudsen and others added 2 commits January 27, 2020 17:22

Small tweaks for types.

964db16

Update .eslintrc.js

e29a1f4

Co-Authored-By: Sean Poulter <[email protected]>

seanpoulter reviewed Jan 27, 2020

View reviewed changes

connectdotz closed this Feb 8, 2020

		@@ -1,14 +1,15 @@
		/* eslint-disable @typescript-eslint/no-use-before-define */
		/* eslint-disable max-classes-per-file */

Typescript conversion #32

Typescript conversion #32

Conversation

rossknudsen commented Jan 25, 2020 • edited Loading

rossknudsen commented Jan 25, 2020

rossknudsen commented Jan 26, 2020

connectdotz commented Jan 26, 2020

stephtr commented Jan 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossknudsen Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr 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

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

rossknudsen commented Jan 26, 2020

rossknudsen commented Jan 27, 2020

seanpoulter commented Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

rossknudsen commented Jan 27, 2020

connectdotz commented Jan 27, 2020

connectdotz commented Feb 8, 2020

rossknudsen commented Jan 25, 2020 •

edited

Loading

stephtr Jan 26, 2020 •

edited

Loading

rossknudsen Jan 26, 2020 •

edited

Loading

seanpoulter commented Jan 27, 2020 •

edited

Loading