Convert LSP packages to TS (WIP) #957

acao · 2019-09-14T23:41:24Z

- graphql-language-service-utils
- graphql-language-service-types
- graphql-language-service-parser
- graphql-language-service-interface
- shared tooling for composite build
- update linting/etc scripts for typescript, add typescript-eslint, etc

benjie · 2019-09-16T10:36:42Z

package.json

    "lint-check": "eslint --print-config .eslintrc.js | eslint-config-prettier-check",
    "check": "flow check --show-all-errors",
    "prepublish": "node resources/prepublish.js",
    "pretty": "node resources/pretty.js",
    "pretty-check": "node resources/pretty.js --check",
    "version:release": "lerna version",
-    "version:prerelease": "lerna version --conventional-prerelease",
+    "version:prerelease": "lerna versiton --conventional-prerelease",


benjie

Couple comments; since it's WIP I'll stop here.

packages/graphql-language-service-interface/src/GraphQLLanguageService.ts

packages/graphql-language-service-interface/src/__tests__/getDefinition-test.ts

acao · 2019-09-16T11:50:20Z

another big commit on the way, down to about 52 compilation errors across all the packages!

- added fix and comment for new failing tests - added assertions for new tests for Jest - switched back to global jest run, 12.8s for all but codemirror! woo! - make sure mocks arent being built by babel as well (fixes warning)

acao · 2019-09-24T20:57:02Z

oh so this weekend it was fully compiling, tests are mostly working too, just having an issue with the typescript and babel eslint parsers butting heads. We will probably have to execute the eslint bin twice. Theres actually an entire repo devoted to this issue. So this won't be complete until all the tooling supports both flow and typescript! @Neitsch if you wanted to finish/polish up be my guest!

benjie · 2019-09-25T09:56:28Z

@acao I was handling this same problem in the Graphile codebase on Monday; I've managed to make ESLint, Flow and TypeScript play fairly well together. Here's my ESLint config in case it helps:

https://github.com/graphile/graphile-engine/blob/bc047b2f86a187c6aa3621d8cd4f423f6cbaa2fc/.eslintrc.js

One thing of note is that you cannot do extends in the overrides so you have to list the rules manually.

Neitsch · 2019-09-28T16:02:39Z

Thanks @benjie - with that in hand it was super easy to resolve the lint errors. Let's see what travis has to say :)

benjie · 2019-09-28T18:31:41Z

Awesome @Neitsch I'm glad it was helpful. It certainly took me a little while to get it working smoothly!

CI: looks like there's issues resolving the packages (yarn workspaces should have handled this for us?)

acao · 2019-09-28T20:44:56Z

damn @benjie thanks for that babel eslint + ts eslint parser solution! i spent a few hours flummoxed on that. maybe we should let this poor soul know because they have a whole repo devoted to trying to resolve this issue.

acao · 2019-09-28T20:47:49Z

also, thanks y'all in general for the save! please don't wait for me if you want to carry this to completion. also @rebornix on dischord from vscode might be able to give a review for this or later iterations to point out which official vscode types we might be able to leverage, reducing redunancies and making us more standard adherent i think?

acao · 2019-09-28T20:50:36Z

also @orta what do you think? i used types quite liberally instead of interfaces mostly because i was bein kinda lazy about converting them from flow, and types seemed appropriate most of the time. Any chance that you might see some of them as a type smell? I guess that really depends on how users plan on using and extending the types. And then any other issues in terms of official typescript best practices?

acao · 2019-09-29T05:45:10Z

packages/graphql-language-service-interface/src/__tests__/getAutocompleteSuggestions-test.ts

@@ -0,0 +1,321 @@
+/**


noting the path for these files wasn't rewritten, and there really weren't that many changes. not a huge deal also not ideal, ope

packages/graphql-language-service-interface/src/__tests__/GraphQLLanguageService-test.ts

Neitsch · 2019-09-30T15:30:53Z

@acao - I was able to resolve the flow type issues, but tests are failing now. Do you have any idea what could be going wrong here?

acao · 2019-09-30T15:51:53Z

@Neitsch I’m hoping to have time to look tomorrow, looks like some typescript errors from the tests themselves? Were these working before?

Neitsch · 2019-09-30T15:57:40Z

@acao - it looks like these errors are stemming from ts-jest performing some diagnistics. I threw in ts-ignore where applicable, but some errors are just weird and I don't quite know what caused those

acao · 2019-10-01T14:40:56Z

@Neitsch try this in the jest config:

projects: ["packages/*"]

so that the downstream subpackages become the baseDir and their tsconfigs are picked up. Last time I had issues like this, this resolved quite a few of them. Esp in situations where the ts compile works fine, but then suddenly ts jest is throwing a bunch of typescript compilation errors. There might be a little bit more fine tuning too.

I really hope I have time to look at this and the accessibility PR tonight but it all depends on whether a soon to be ex roomate decides to get around to cleaning out their stuff tonight, unfortuantely it seems very unlikely at this point.

codecov · 2019-10-02T15:56:20Z

Codecov Report

Merging #957 into master will decrease coverage by 6.48%.
The diff coverage is 37.5%.

@@            Coverage Diff            @@
##           master    #957      +/-   ##
=========================================
- Coverage   33.78%   27.3%   -6.49%     
=========================================
  Files          79      64      -15     
  Lines        3481    2641     -840     
  Branches      752     566     -186     
=========================================
- Hits         1176     721     -455     
+ Misses       1814    1503     -311     
+ Partials      491     417      -74

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/variables/lint.js	`0% <ø> (ø)`	⬆️
packages/graphiql/src/components/ToolbarSelect.js	`6.45% <ø> (ø)`	⬆️
...ackages/codemirror-graphql/src/utils/jump-addon.js	`0% <ø> (ø)`	⬆️
packages/graphiql/src/utility/find.js	`33.33% <ø> (ø)`	⬆️
...es/codemirror-graphql/src/utils/SchemaReference.js	`0% <ø> (ø)`	⬆️
packages/graphiql/src/components/ResultViewer.js	`68.57% <ø> (ø)`	⬆️
packages/graphql-language-service/src/client.js	`0% <ø> (ø)`	⬆️
packages/codemirror-graphql/src/jump.js	`0% <ø> (ø)`	⬆️
packages/graphiql/src/utility/mergeAst.js	`100% <ø> (ø)`	⬆️
packages/graphiql/src/utility/onHasCompletion.js	`3.12% <ø> (ø)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6b0e50...9e97a1d. Read the comment docs.

acao · 2019-10-03T14:20:03Z

sadly, had to do a merge commit, because there were other merge commits in the master branch upstream. If we stop doing merge commits then we can rebase much more easily!

acao · 2019-10-03T14:25:41Z

I'm getting these errors again after merging in the upstream? looks like we aren't linting TS anymore?

acao · 2019-10-03T14:27:50Z

we are using eslint 6.4 now in master:
https://github.com/graphql/graphiql/blob/master/package.json#L54

in this branch we still have 5.16.0 after i merged remote in?:

graphiql/package.json

Line 56 in ea78946

"eslint": "^5.16.0",

something funky here

acao · 2019-10-03T14:29:54Z

@benjie @Neitsch i have to get to work now, any chance you guys can look at this today? if not ill keep at it tomorrow. from this branch locally I did git merge master -Xours and i think we got some issues from that.

Neitsch · 2019-10-03T16:58:23Z

CI is failing. Would you mind updating the yarn lockfile?

acao · 2019-10-04T02:33:16Z

@Neitsch unfortunately its more than updating the lockfile, if you see the erroneous eslint warnings above, shows that @typescript-eslint/parser is not working

Neitsch · 2019-10-04T08:00:09Z

I don't know where exactly in this PR it happened, but I would strongly prefer to keep trailing commas around!

Either way, awesome that it's finally merged!

acao force-pushed the lsp-ts branch 4 times, most recently from d137731 to 6878d7c Compare September 15, 2019 00:26

benjie reviewed Sep 16, 2019

View reviewed changes

packages/graphql-language-service-interface/src/GraphQLLanguageService.ts Outdated Show resolved Hide resolved

packages/graphql-language-service-interface/src/__tests__/getDefinition-test.ts Outdated Show resolved Hide resolved

chore: Convert LSP tests to jest from mocha (#871)

415a957

- added fix and comment for new failing tests - added assertions for new tests for Jest - switched back to global jest run, 12.8s for all but codemirror! woo! - make sure mocks arent being built by babel as well (fixes warning)

acao force-pushed the lsp-ts branch 2 times, most recently from ac5c875 to 427f27c Compare September 17, 2019 02:48

chore: more ts conversion

1d42bfe

acao force-pushed the lsp-ts branch from 427f27c to 1d42bfe Compare September 21, 2019 23:07

chore: make eslint work with typescript

f8b89ae

acao commented Sep 29, 2019

View reviewed changes

packages/graphql-language-service-interface/src/__tests__/GraphQLLanguageService-test.ts Outdated Show resolved Hide resolved

chore: add flowtypes for TS packages

ffb6170

chore: ignore some TS errors in tests

b5b8447

Neitsch added 2 commits October 2, 2019 11:16

chore: fix failing tests

da4619f

chore: fix remaining failing testcase

4bddcee

chore: module resolution commonjs

c3ad249

Merge branch 'master' into lsp-ts

ea78946

chore: fix eslint

9e97a1d

acao merged commit 36ed669 into master Oct 4, 2019

acao deleted the lsp-ts branch November 20, 2019 04:40

acao added the a11y label Feb 20, 2020

tnrich mentioned this pull request Jun 2, 2020

regressions in variable validation #1565

Closed

dimitropoulos mentioned this pull request May 19, 2021

We're moving to TypeScript! 🎉 Kong/insomnia#3266

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert LSP packages to TS (WIP) #957

Convert LSP packages to TS (WIP) #957

acao commented Sep 14, 2019 •

edited

Loading

benjie Sep 16, 2019

benjie left a comment

acao commented Sep 16, 2019

acao commented Sep 24, 2019 •

edited

Loading

benjie commented Sep 25, 2019

Neitsch commented Sep 28, 2019

benjie commented Sep 28, 2019

acao commented Sep 28, 2019

acao commented Sep 28, 2019

acao commented Sep 28, 2019 •

edited

Loading

acao Sep 29, 2019 •

edited

Loading

Neitsch commented Sep 30, 2019

acao commented Sep 30, 2019

Neitsch commented Sep 30, 2019

acao commented Oct 1, 2019

codecov bot commented Oct 2, 2019 •

edited

Loading

acao commented Oct 3, 2019

acao commented Oct 3, 2019

acao commented Oct 3, 2019

acao commented Oct 3, 2019

Neitsch commented Oct 3, 2019

acao commented Oct 4, 2019

Neitsch commented Oct 4, 2019

Convert LSP packages to TS (WIP) #957

Convert LSP packages to TS (WIP) #957

Conversation

acao commented Sep 14, 2019 • edited Loading

benjie Sep 16, 2019

Choose a reason for hiding this comment

benjie left a comment

Choose a reason for hiding this comment

acao commented Sep 16, 2019

acao commented Sep 24, 2019 • edited Loading

benjie commented Sep 25, 2019

Neitsch commented Sep 28, 2019

benjie commented Sep 28, 2019

acao commented Sep 28, 2019

acao commented Sep 28, 2019

acao commented Sep 28, 2019 • edited Loading

acao Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

Neitsch commented Sep 30, 2019

acao commented Sep 30, 2019

Neitsch commented Sep 30, 2019

acao commented Oct 1, 2019

codecov bot commented Oct 2, 2019 • edited Loading

Codecov Report

acao commented Oct 3, 2019

acao commented Oct 3, 2019

acao commented Oct 3, 2019

acao commented Oct 3, 2019

Neitsch commented Oct 3, 2019

acao commented Oct 4, 2019

Neitsch commented Oct 4, 2019

acao commented Sep 14, 2019 •

edited

Loading

acao commented Sep 24, 2019 •

edited

Loading

acao commented Sep 28, 2019 •

edited

Loading

acao Sep 29, 2019 •

edited

Loading

codecov bot commented Oct 2, 2019 •

edited

Loading