[lsp-server] 🐞 incorrect bundling of types for `graphql-language-server`

[phryneas]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current Behavior

Currently, graphql-language-server@5.3.0 causes type errors with graphql@16.9.0. Apparently, this package bundles the types of the development dependency graphql into the published bundle, which will then cause TypeScript errors.

Also see the discussion in https://github.com/graphql/graphql-js/issues/4201

Expected Behavior

As 16.9.0 is a valid peerDependency, it should not result in TS warnings.

Steps To Reproduce

Install graphql-language-server@5.3.0 and graphql@16.9.0, run tsc.

Environment

See this PR: https://github.com/apollographql/vscode-graphql/pull/175 with a failing CI job

Anything else?

No response

[yaacovCR]

This would be fixed by the latest release: https://github.com/graphql/graphiql/pull/3763

[phryneas]

I don't think that a "move back to 16" is necessarily required to fix this. (I think that's what you're hinting at, right?)

The problem is that the graphql types end up in the graphql-language-server package - not as an imported dependency, but a rolled-up type file.

[phryneas]

Or no, looking at the source code, I think this might actually not be the case and @JoviDeCroock might have overinterpreted things. The problem is not necessarily the bundling itself, but this statement:

export const RuleKinds = {
  ...Kind,
  ...AdditionalRuleKinds,
};

The spread pulls in all the information about Kind.

[JoviDeCroock]

That spread, even if it's used as a JS-value, can be imported from graphql, which it does in JS but somehow in TypeScript rather than transpiling to export type RuleKinds = Kind & Y it's exploding the values. That being said, I am not really in the boat of that being an over-interpretation 😅 as the described state in the original issue is identical to the concluded state of the foregoing comment. This does show a cause though, the usage of Type enums has been flakey at times.

[phryneas]

Yeah, I meant it's not a problem on the build tooling side, which I had assumed you meant.

I think this could fix it, I'm trying it out as we speak:

export const RuleKinds: Omit<typeof Kind, keyof _AdditionalRuleKinds> &
  _AdditionalRuleKinds = {
  ...Kind,
  ...AdditionalRuleKinds,
};

[phryneas]

Oh, even simpler...

-export const RuleKinds = {
+export const RuleKinds: _RuleKinds = {
  ...Kind,
  ...AdditionalRuleKinds,
};

this would fix it without having to roll back the version of graphql (unless there is another reason)

[yaacovCR]

I believe we wanted to move back to v16 as the dev dependency because it’s actually re-exported in one of the packages in total, and when we upgraded, we did it under the assumption that it would not affect users at all.

So we are definitely moving back to v16, which can be released whenever appropriate. @acao please feel free to jump in if I have that wrong.

More broadly, things could be rearchitectured of course!

[yaacovCR]

graphiql will still support v17, but it won’t be the version in the dev dependency and I assume will fix this problem.

[phryneas]

I'm happy either way :)