Handling vscode-languageserver with NMR

[MrSnix]

The problem:
While working on Insomnia, this warning message gets usually displayed every time the app is recompiled or simply reloaded.

Now, for the sake of it, I googled around to better understand if we can do something about it.

Webpack doesn't like the way the require function is used inside this specific package (vscode-languageserver-types).

Going deeper, Webpack doesn't like the UMD (Universal Module Definition) format, it gladly accepts commonJS, AMD and ESM but not UMD, the native format of the mentioned package. So he throws the warning.

When using webpack to bundle your application, you can pick from a variety of module syntax styles including ES6, CommonJS, and AMD.

Source: Webpack v4 Doc

The 1st solution:
Using noParse inside the configuration prevents webpack from parsing any files matching the given regular expression(s) and from throwing this annoying issue. vscode-languageserver-types is recognised to be non-webpack friendly due to his format and it's harmless ignoring him. It won't change its behaviour at runtime.

module: {
    noParse: [/node_modules\/vscode-languageserver-types/],
}

The 2nd solution
Another possible workaround is to deal with it using umd-compat-loader.
This loader simply removes the UMD wrapper - effectively unwrapping it back to commonjs.

{
    test: /node_modules\/vscode-/,
    use: {loader: "umd-compat-loader"}
}

So, this PR ?:
I propose to fix this with the second approach, allowing webpack to statically extract the dependencies because I feel like it is more consistent instead of ignoring it ... but as always, every opinion is welcomed.

Proof:
No more warning is displayed.

Doc & Issue:
vscode-languageservice thread
Webpack - No parse

[develohpanda]

Seems like a reasonable explanation and does what it says. I was curious and it appears this package is a downstream dependency of codemirror-graphql.

Thanks for fixing this up! 🙌🏽

[dimitropoulos]

@MrSnix I really value your contributions. thank you! this was bugging me too, just hadn't had the time+headspace to look into it. Thanks for writing up your process.

[dimitropoulos]

@MrSnix I'd be kinda curious if it's possibly to easily close the loop on this (and remove the need for us to add another loader) in the future. It appears according to microsoft/vscode-languageserver-node#303 (comment) that vscode-languageserver-node have allowed ES Modules for a few years now. I wonder what change would be required to get that version into our build. I think maybe it'd be a change in graphql-language-service-interface since that's the one that immediately uses the vscode-languageserver-types package? https://github.com/graphql/graphiql/blob/main/packages/graphql-language-service-interface/package.json#L33

[dimitropoulos]

Just to see... I tried updating graphql to v15.5.1 to see if it makes the error go away, and it didn't. @MrSnix did you ever get a chance to take a look (no pressure whatsoever! just curious if you had any thoughts 😄)?

[develohpanda]

Reopening this because it seemed to have been closed in error with #3926 😅

[MrSnix]

Hi @develohpanda and @dimitropoulos, I apologize for the late answer on this one 😭 .

The 3rd solution
I was able to replace the umd import using the NormalModuleReplacementPlugin provided by Webpack (no additional deps 😊 ) . I'm basically rewriting the import forcing the usage of the esm package. Seems working as expected. I don't know if it's an acceptable solution, if someone more experienced can tell me if there may be further problems (that I'm not aware of), please let me know.

Is it what you were looking for @dimitropoulos 😃 ?

Changes:

• Fix conflicts
• Reverted umd-loader import
• Now using NormalModuleReplacementPlugin

[dimitropoulos]

that's certainly an improvement! I'll take it (don't get me wrong!) but what I was thinking about was fixing this in the upstream such that no changes other than updating a dependency in our package.json(s) would be needed to get this error to go away.

it seems like this could be a change that needs to be made to graphql-language-service-interface maybe?

I'll take a look at this again tomorrow, but as far as I'm concerned this is pretty much ready to merge. Can't have our cake and eat it too. Still might be cool to look into the upstream to see why this is happening.

[MrSnix]

Okay @dimitropoulos , let's sort this out 😎 !

it seems like this could be a change that needs to be made to graphql-language-service-interface maybe?

If I am not wrong, the main issue is related to the entry point specified by vscode-languageserver-types inside its own package.json which purposely uses the umd flavour 🤓 . I think it has nothing to do with the graphql-language-service-interface.

Here you can see it: vscode-languageserver-types - package.json - L15

{
    "main": "./lib/umd/main.js",
    "typings": "./lib/umd/main"
}

I was thinking about was fixing this in the upstream such that no changes other than updating a dependency in our
package.json(s) would be needed to get this error to go away.

We may ask for changing the entry point 🧐 but I don't think authors are willing to do it 😬 as they already stated in another thread.

There is a reason that we compile this to UMD.
It is because the types package is used in the Monaco editor right.
Can you please take this?

This is a quote from: compile types package to commonjs module

The proposed solution inside those threads is the umd-loader or to replace the library with a different output (ESM) inside our building process. (as I did with webpack NMR)

What do you think ?
Let me know 🚀

Thanks

[dimitropoulos]

The reason that vscode-languageserver-types exists in our project is because of codemirror-graphql which uses grapql-language-service-interface which uses a UMD version of vscode-languageserver-types. I was thinking that the error came from the graphql thing not using esm and maybe if we switched its usage of vscode-languageserver-types to esm then the problem would go away for us.

I don't think authors are willing to do it

So, to recap, you're totally right about this, but it looks like we're both talking different npms. What I'm sayin' is I think we can fix the problem higher up the dependency tree at the level of the graphql dependencies.

While lookin around I found something very interesting - https://github.com/graphql/graphiql/tree/main/packages/graphql-language-service-interface seems to have an esm version (notice, tsconfig.esm.json), and so does https://github.com/graphql/graphiql/tree/main/packages/codemirror-graphql#readme (notice, same file in this package).

Looking further up their monorepo, it seems like they're going with esm for everything, 2 years ago they made a commit graphql/graphiql@34101c8 which I only saw because it's the most recent change to their monorepo's root tsconfig.

I'm still looking into it, but maybe we can just change how we consume codemirror-graphql and this whole thing will go away?

Normally I'd look for that and wait to post this comment, but I can see that you are working on this now so I wanted to send this to you as I found it. I'm gonna spend a few more minutes on this and report back some time today one way or another what I find.

---

by the way, the solution you found with NormalModuleReplacementPlugin is great! fantastic find!! If we don't find another easy way to do this by managing our graphql deps then I'd be very happy with this.

[dimitropoulos]

ok, here's an idea, we may be able to tell TypeScript to just get the esmodule version (if it isn't already, and I feel like the fact that we're getting this error is evidence that it isn't, maybe?): https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping.

Ok, so that just means adding to our tsonfig for insomnia-app. Shouldn't be so difficult, right? Well, haha.. apparently not so:

I don't quite understand how, but the package.json of codemirror-graphql is pointing to files that don't appear to exist (index.js and (esm/index.js). Maybe that's the problem here.

Thoughts?

[dimitropoulos]

Another update: I can confirm that this error does indeed stem from codemirror-graphql. I didn't think to look sooner, but there's only one file where we're using it, https://github.com/Kong/insomnia/blob/develop/packages/insomnia-app/app/ui/components/codemirror/base-imports.ts#L46-L52.

If you remove those imports, the error goes away.

import 'codemirror-graphql/hint';
import 'codemirror-graphql/lint';
import 'codemirror-graphql/info';
import 'codemirror-graphql/jump';
import 'codemirror-graphql/mode';
import 'codemirror-graphql/variables/lint';
import 'codemirror-graphql/variables/mode';

I tried changing these like this:

import 'codemirror-graphql/hint.esm';
import 'codemirror-graphql/lint.esm';
import 'codemirror-graphql/info.esm';
import 'codemirror-graphql/jump.esm';
import 'codemirror-graphql/mode.esm';
import 'codemirror-graphql/variables/lint.esm';
import 'codemirror-graphql/variables/mode.esm';

and also like this

import 'codemirror-graphql/hint.esm.js';
import 'codemirror-graphql/lint.esm.js';
import 'codemirror-graphql/info.esm.js';
import 'codemirror-graphql/jump.esm.js';
import 'codemirror-graphql/mode.esm.js';
import 'codemirror-graphql/variables/lint.esm.js';
import 'codemirror-graphql/variables/mode.esm.js';

and these didn't compile.

Ok, so then I also tried adding

    "baseUrl": "./",
    "paths": {
      "codemirror-graphql/hint": ["codemirror-graphql/hint.esm"],
      "codemirror-graphql/lint": ["codemirror-graphql/lint.esm"],
      "codemirror-graphql/info": ["codemirror-graphql/info.esm"],
      "codemirror-graphql/jump": ["codemirror-graphql/jump.esm"],
      "codemirror-graphql/mode": ["codemirror-graphql/mode.esm"],
      "codemirror-graphql/variables/lint": ["codemirror-graphql/variables/lint.esm"],
      "codemirror-graphql/variables/mode": ["codemirror-graphql/variables/mode.esm"],
    }

to the insomnia-app/tsconfig.build.json's compilerOptions. That, too, didn't make the error go away. I stopped with this line of attack, but wanted to share what I tried because maybe it'll spring an idea for someone else.

So the three interesting pieces of information are:

1. codemirror-graphql is publishing esmodules but we aren't using them
2. the codemirror-graphql package has package.json values main and module that aren't actual files (this seems like a bug to me)
3. the actual place where we import from codemirror-graphql we import specific files (like their docs do https://github.com/graphql/graphiql/tree/main/packages/codemirror-graphql)

[dimitropoulos]

one other finding:
codemirror-graphql is only importing from graphql-language-service-types for the sake of a single Maybe type.. No source code, just the TypeScript type. I wonder if this means we can just exclude this from TypeScript alltogether and make the error (which, after all, is from a dependency (vscode-languageserver-types) of a dependency (graphql-language-service-types) of a dependency (codemirror-graphql)) go away

[MrSnix]

I never dealt with an issue like this, so I'm really going blind on this one and I may say dumb things 😭 , be aware!

We may be able to tell TypeScript to just get the es-module version (if it isn't already, and I feel like the fact that we're getting this error is evidence that it isn't, maybe?)

I agree with you 😄 , I think it's not using the right version 😢 .

My unsuccessful attempt:
I was trying to do something more simple instead of using the path-mapping... like forcing the usage of the ESM inside the base-imports.ts but I wasn't successful at all.

The root cause
The culprit of the warning seems located inside this import: 'codemirror-graphql/lint.js';

[dev] WARNING in ../node_modules/vscode-languageserver-types/lib/umd/main.js 3:24-31
[dev] Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
[dev]  @ ../node_modules/graphql-language-service-interface/dist/getAutocompleteSuggestions.js
[dev]  @ ../node_modules/graphql-language-service-interface/dist/index.js
[dev]  @ ../node_modules/codemirror-graphql/lint.js
[dev]  @ ./ui/components/codemirror/base-imports.ts

It uses the following snippet to load the graphql-language-service-interface:

File: lint.js

  var graphql_language_service_interface_1 = require("graphql-language-service-interface");

I assume <= this is the reason the umd version is loaded

Meanwhile the esm version:

File: lint.esm.js

import { getDiagnostics } from 'graphql-language-service-interface';

The idea
I thought that if I force the import with the ESM I could be able to get away with it but Typescript is not transpiling the import and webpack don't recognise the new syntax like the ?. optional chaining operator.

Failed to compile, webpack

[dev] ERROR in ../node_modules/codemirror-graphql/variables/lint.esm.js 28:44
[dev] Module parse failed: Unexpected token (28:44)
[dev] You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
[dev] |     variablesAST.members.forEach(member => {
[dev] |         if (member) {
[dev] >             const variableName = member.key?.value;
[dev] |             const type = variableToType[variableName];
[dev] |             if (!type) {
[dev]  @ ./ui/components/codemirror/base-imports.ts 72:0-48
[dev]  @ ./ui/components/codemirror/code-editor.tsx
[dev]  @ ./ui/components/modals/generate-code-modal.tsx
[dev]  @ ./ui/containers/app.tsx
[dev]  @ ./ui/index.tsx
[dev]  @ ./renderer.ts
[dev]  @ multi webpack-dev-server/client?http://localhost:3334 webpack/hot/only-dev-server ./renderer.ts ./renderer.html

Considerations:

1. I still don't grasp if the mistake is caused by Typescript transpilation or by Webpack (I think Webpack ?)
2. Assuming Typescript would have transpiled the ESM module and webpack would have run... would have been enough to load the ESM version and make the warning go away?

I don't know if my reasoning is right but my knowledge ends here 😢 and I don't think I can help more than that.
I'll be watching 🧐 this issue with great interest to see how this is going to be solved 🟢 ... if any (or any future pull request).

EDIT:
@dimitropoulos You are too fast boy 😆 ... well apparently we had the same idea.

[MrSnix]

Another consideration about paths-mapping:

It seems not effective, I mean it seems like it doesn't do anything at all.

"paths":` {
      "codemirror-graphql/hint": ["codemirror-graphql/hint.esm"],
      "codemirror-graphql/lint": ["codemirror-graphql/lint.esm"],
      "codemirror-graphql/info": ["codemirror-graphql/info.esm"],
      "codemirror-graphql/jump": ["codemirror-graphql/jump.esm"],
      "codemirror-graphql/mode": ["codemirror-graphql/mode.esm"],
      "codemirror-graphql/variables/lint": ["codemirror-graphql/variables/lint.esm"],
      "codemirror-graphql/variables/mode": ["codemirror-graphql/variables/mode.esm"],
    }

In the warning message I still get this stack trace:

[dev] WARNING in ../node_modules/vscode-languageserver-types/lib/umd/main.js 3:24-31
[dev] Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
...
[dev]  @ ../node_modules/codemirror-graphql/lint.js
...

Meanwhile, I'd expect this:

[dev] WARNING in ../node_modules/vscode-languageserver-types/lib/umd/main.js 3:24-31
[dev] Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
...
[dev]  @ ../node_modules/codemirror-graphql/lint.esm.js <=
...

[dimitropoulos]

Welp. I'm outta ideas for things we can do in our project. I learned a lot, so that's always cool, but I don't think it's worth the requisite time to really get this solved in the cleanest possible way for our own project.

I was moreso interested in better understanding this because I want to know if there's some issue in our build chain and this is just the first/only place that happens to surface an error. That doesn't seem to be the case - it looks like something specific to this one dependency (graphql-language-service-interface) and how it imports vscode-languageserver-types.

Not totally mandatory, but I think we should add a comment (https://github.com/Kong/insomnia/pull/3469/files#r692320932) linking back to this PR, should someone wonder in the future (or if this line is moved somewhere else and it gets hard to track the git history in the future) what this was all about.

@develohpanda what do you think?

Once we merge, I'll make an issue (unless @MrSnix you want to) on the graphql-language-service-interface just to let them know about our use-case. Who knows, maybe they'll have a better solution. I searched their repo and didn't see anyone else that has reported this.

[MrSnix]

@dimitropoulos I was thinking that you may be right about the package.json pointing to the wrong path.
I think they may have some kind of misconfiguration in their building publishing process related to the codemirror-graphql package and the folder is not getting published.

A lot of their modules have the ESM folder exported but this one... well it doesn't.

Some examples:

1. codemirror-graphql (bug, no ESM folder exported)
   https://unpkg.com/browse/codemirror-graphql@1.0.2/

2. graphql-language-service-interface (yes, ESM folder exported)
   https://unpkg.com/browse/graphql-language-service-interface@2.8.4/

3. graphql-language-service-parser (yes, ESM folder exported)
   https://unpkg.com/browse/graphql-language-service-parser@1.9.2/

4. graphql-language-service-server (yes, ESM folder exported)
   https://unpkg.com/browse/graphql-language-service-server@2.6.3/

About who should open the issue, I leave the choice to you or @develohpanda.
If any, I'm still willing to open it. Let me know.

EDIT:
It seems that the ESM files are scattered within the root folder rather than in the /esm sub-folder

EDIT 2:
I forked the codemirror-graphql package and I built it according to their configuration and the esm folder is correctly created ... but this doesn't reflect the published version on the npm registry.

NPM Registry: https://unpkg.com/browse/codemirror-graphql@1.0.2/

My build:

[dimitropoulos]

@MrSnix great find! make an issue and show them what you found and let's see what they say. maybe that's the whole issue(?!?!)?

[MrSnix]

@dimitropoulos I opened the issue, if necessary please feel free to add more context, I limited myself to describe and contextualize the bug without explaining the whole Webpack thing related to our warning: See the new issue here

[dimitropoulos]

It looks like they go months without responding to issues. I think we should just merge this for now. @develohpanda if you agree, please merge.