JetBrains / js-graphql-intellij-plugin

GraphQL language support for WebStorm, IntelliJ IDEA and other IDEs based on the IntelliJ Platform.
https://jimkyndemeyer.github.io/js-graphql-intellij-plugin/
MIT License
879 stars 97 forks source link

Scan for schema files in libraries #498

Closed paulbakker closed 2 years ago

paulbakker commented 2 years ago

At Netflix we have Java libraries with "common" GraphQL types. These types are shared by many GraphQL services. The libraries contain a schema in META-INF/schema and can optionally also contain code.

When writing a schema for a service, these types can be used just like types defined in the project. Scalar types are a common example. The problem is that the plugin doesn't recognize the types defined in the schema files provided by libraries, so it rightfully complains in the editor that the types are undefined.

This PR changes the search scope for schema files to allScope instead of projectScope, which makes it include libraries as well. The following is an example of how that works.

ScreenFlow

The schema in JAR file mechanism is supported by the DGS framework, which is getting quite popular, so it's a mechanism that will likely benefit many users. I understand the plugin has been more JS-focused, but it's definitely just as useful for Java developers.

jimkyndemeyer commented 2 years ago

Hi Paul,

Thanks for the PR.

Happy to hear that Netflix is getting value out of the plugin.

I originally used the project files scope because I had concerns about including node_modules which can be upwards of 100.000+ files in large JS projects.

@vepanimas What are your recommendations here? Should it be an option in the plugin? Maybe a dropdown with ("Project files", "Project files and library files", "All places")? I've seen a number of other users requesting that .graphql files in node_modules should be included in schema discovery. I assume that the indexing is quite fast based on file type filtering alone, but the plugin also indexes all JS/TS files for tagged template literals which would then include node_modules.

vepanimas commented 2 years ago

Hi! I'll take a closer look tomorrow. Such changes generally bother me, because I've seen some tickets and reviews that the plugin's performance is not that great on large projects. And using a scope of this kind can potentially worsen it. At least, I would like to be one hundred percent sure that we include only GraphQL files in this scope (no JSONs and JS/TS/etc files with injections), maybe it would be better to create a specific restricted library scope and union it with the existing project scope.

@paulbakker could you please share an example project with DGS framework for testing?

paulbakker commented 2 years ago

Thanks for the quick response!

I've made some changes to be more specific what/where to search to prevent the potential performance implications. My understanding is that Intellij indexes all files anyway (at startup), which would mean the search scope shouldn't impact performance much. However, I really don't know the details, and being more conservative where to search is obviously safer.

In the new commit, I've added a new SchemaInLibraryScope, which does the following:

I'm using this scope to union with the existing scopes.

Also, I've updated one of our DGS examples to use a type from a library: https://github.com/Netflix/dgs-examples-java/tree/schema-in-lib-example. Make sure you check out the linked branch.

paulbakker commented 2 years ago

Thanks for the review! I'll make the changes when I'm back online Wednesday.

paulbakker commented 2 years ago

I've made the suggestions from @vepanimas. Thanks for that, I basically just copy/pasted what you suggested :-)

For the class name of the new scope, I renamed it to MetaInfSchemaScope, which I think is a little more specific than JarResourcesLibraryScope. I don't have a strong opinion about the name, so happy to change it.

paulbakker commented 2 years ago

Thanks for getting this in! It's going to be a huge help for our engineers.