search

graphql / vscode-graphql

MIGRATED: VSCode GraphQL extension (autocompletion, go-to definition, syntax highlighting)
https://marketplace.visualstudio.com/items?itemName=Prisma.vscode-graphql
MIT License
557 stars 71 forks source link

chore: bump graphql-language-service-server #335

Closed ferm10n closed 2 years ago

ferm10n commented 2 years ago
linux-foundation-easycla[bot] commented 2 years ago

CLA Signed

The committers are authorized under a signed CLA.

ferm10n commented 2 years ago

curious, does this project still need a yarn.lock? wasn't sure if I should update that as well

acao commented 2 years ago

Yes it has a yarn lock. So there should be a diff to the yarn lock to make thid mergeable. Not sure why codemirror types are used though? That has to be a bug

ferm10n commented 2 years ago

This import is what causes the build to fail without the codemirror dep https://github.com/graphql/graphiql/blob/672aadf121d47f53d115fb2de5446039d66d684b/packages/graphql-language-service-interface/src/getAutocompleteSuggestions.ts

and yeah I'll update the yarn.lock. I was just confused why there were both types of lockfiles, and the last few releases only modified the package-lock (most recent https://github.com/graphql/vscode-graphql/commit/4313525252ab7225b061e2ed11be9cba3ca8ac40#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519)

acao commented 2 years ago

Oh my bad, this repo still uses package lock. I was thinking of the lsp monorepo. Need to merge this repo in with the other projects!

acao commented 2 years ago

@ferm10n can you help me out by making another PR to the LSP repo (graphiql) to replace the codemirror types used with custom ones? The LSP should not depend on codemirror types, I did not notice this addition, hmm. The context token type was originally in the types package

ferm10n commented 2 years ago

@acao okay I think this should do it! https://github.com/graphql/graphiql/pull/1998

assuming you want to get that merged in and released, and this PR updated to use the new release, are there any other changes needed?

acao commented 2 years ago

@ferm10n can you update to 2.7.1? Just published a bunch of new versions.

ferm10n commented 2 years ago

Okay so I did that, and then started getting 49 errors from the graphql-language-service-server package. I've pushed up the commit if you want to check it out.

Seems like the usual graphql related type issues 😖 All the errors fall into one of these three types

node_modules/graphql-language-service-server/esm/MessageProcessor.d.ts:4:49 - error TS2307: Cannot find module '@graphql-tools/load' or its corresponding type declarations.

import type { UnnormalizedTypeDefPointer } from '@graphql-tools/load';
node_modules/graphql-language-service-parser/dist/types.d.ts:105:11 - error TS2503: Cannot find namespace 'Kind'.

NAME: Kind.NAME;

also, different versions of graphql-config are being used with conflicting types

I'll try to tinker with it a bit and see if there's an easy fix. Any recommendations would be really appreciated

ferm10n commented 2 years ago

NOTE: The following only focuses on ONE of the main problems mentioned earlier

based on this, I'd say that graphql-language-service-parser is not compatible with the graphql@15 types anymore. For laughs, I tried updating vscode-graphql to depend on graphql@16 (and typescript@^4.1.0), and I ended up with only 10 errors, mostly involving apollo-link and graphql-tools...

However, my gut tells me that fighting the dependency hydra is the wrong course of action. I think the dependency on graphql@16 is a breaking change for graphql-language-server-parser, unless we can figure out a way to deal with the enums. If this did result in a new major release (graphql-language-server-parser@2.0.0), I think we'd want to try to stabilize the types in graphiql pre graphql@16... which sounds like another huge task >_< (But maybe one that we can chip away at!)

ferm10n commented 2 years ago

@acao hey what about just adding skipLibCheck to tsconfig? I just remembered that was a thing 😅 It's a bit of a hack though since it'll automatically make types with errors from any d.ts file become an any...

acao commented 2 years ago

Oof! That’s a big problem. We will either need to refactor, or make graphql 16 a requirement in peer dependencies. We are using the defer stream tag until it gets merged in 16.1

acao commented 2 years ago

I did a big series of upgrades including this in #338. I came up with a strategy for us to avoid this issue in graphql@16, but the community peer support for 16 is also not quite there yet, leading to tons of peer dependency issues

acao commented 2 years ago

thank you! this has been accomplished in #338

acao commented 2 years ago

@ferm10n are you still interested in helping with this? the PR to revert that enum to a const is still waiting: https://github.com/graphql/graphql-js/pull/3360

we also build and test the entire graphiql monorepo against 15 and 16 with every commit now, and we haven't seen this issue come up again! strange?

we really don't have a better option, because users were and still need graphql 16 support as well. it should not be a major version change because it's a dependency, we chose a minor version increment instead, unless there's something I'm overlooking with semver

ferm10n commented 2 years ago

@acao It's been a minute, I thought this was done when you closed this. What remains to be done exactly? Also, yes I'm interested in helping but my availability is pretty limited lately so I can't make any promises :/

acao commented 2 years ago

We are in the midst of merging the extension with graphiql repo! There’s an issue with the automatic publishing here

acao commented 2 years ago

Published to vsc marketplace, open vsx still has an issue it would seem