Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pluggable schema loading #920

Closed
wants to merge 2 commits into from
Closed

Pluggable schema loading #920

wants to merge 2 commits into from

Conversation

Neitsch
Copy link
Contributor

@Neitsch Neitsch commented Jul 27, 2019

So, to get us started, I've implemented pluggable schema loading. How this works is the following:

  • We have a new extension key schema-loader that contains two fields loaderPackage and loaderArgs. The loader package will be dynamically loaded. Subsequently we call init(loaderArgs) for it to initialize. (Alternatively we could use ES6 classes)
  • The dynamically loaded package exposes a function getSchema, that if called returns the GraphQL Schema it loaded.

A couple caveats:

  • This PR per se does not add any functionality. It relies on subsequent PRs actually adding schema loaders that conform to the interface.
  • I am not sure wether simply calling require() is a good way to do this, but at least the new test passes
  • This does not add all of the functionality I suggested in the original issue ([RFC][LSP] Pluggable schema loading #915)
  • Right now we instantiate a new schema loader every time. We should probably cache it if it becomes a bottleneck.

@acao
Copy link
Member

acao commented Jul 27, 2019

this is a great, love it! can't wait to implement this with gls-interface + monaco

only caveat of dynamic requires is that they can sometimes break build tooling. for example, common-shake will do a global exit on this, making it so that we can accomplish tree shaking, at least from what I can tell. it might introduce other issues as well, such as making things funky with webpack loaders and how they resolve paths/extensions. see this issue for details #882

@Neitsch
Copy link
Contributor Author

Neitsch commented Jul 27, 2019

Alright, alternative idea how to facilitate this:

global.schemaLoaders = {}
...
if(global.schemaLoaders[mySchemaLoader]) {
  // use dynamic schema loader
}

Then to register a schema loader the developer must import the plugin on a top level. The plugin itself then has some code like this:

global.schemaLoaders['cool-schema-loader'] = {
  init: () => ({ getSchema: () => null })
}

Alternatively, could we mark the loader with sideEffect? (https://webpack.js.org/guides/tree-shaking/)

@Neitsch
Copy link
Contributor Author

Neitsch commented Jul 27, 2019

I guess if we think about an LSP itself it might not actually be able to require other packages that our outside of the extension. So alternatively we could just model this as a command line invocation that prints the loaded schema to stdout. Then we just need to reparse it.

@Neitsch
Copy link
Contributor Author

Neitsch commented Jul 27, 2019

Digging more into that last idea, maybe a platform specific solution is necessary here: For VSCode we communicate via the extensions API (https://stackoverflow.com/questions/50058517/how-to-communicate-between-vscode-extensions). For GraphiQL we communicate via HTTP calls

@acao
Copy link
Member

acao commented Jul 27, 2019

@Neitsch yes 98% of the time via HTTP for graphiql, but I do want the new graphiql to be able to support folks using it in an electron/nwjs/RN?!/etc app and thus being able to use json-rpc alongside alternative, vscode-like schema loading mechanisms. Maybe thats a bit ambitious though, haha!

Either way, I agree that we need to figure out something platform specific for this. Those two patterns are great, and then in my above case, maybe we could read or stream from file or some clever cacheing mechanism. A tool like insomnia or altair, playground desktop would probably be able to leverage this as well since they are using these libraries.

@schickling @imolorhe @gschier @martijnwalraven, thoughts on this interface?

@acao
Copy link
Member

acao commented Jul 27, 2019

possibly we need an interface for platform-specific extensions in general... related to the above issue I mentioned, we somewhere along the idea had the idea of making graphql-config optional via a plugin (that might be a bit ambitious), and here we have the extensibkle schema loader, and Gatsby wants to be able to add their common fragments behind the scenes as well (they were going to do this at the codemirror-graphql level, but I suggested taking it down to the LSP/interface level somehow).

}

getSchemaLoader(): GraphQLSchemaLoader {
return require(this.raw.loaderPackage).init(this.raw.loaderArgs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything about this looks good to me. Love the idea as well. The only part about this that I'm not sure of is this line which you guys were already talking about. I'm not familiar with all the contexts the language server can be used in (for example a browser context might not have the require method), so when this is called at runtime it would need to work well for all contexts it's used in.

Having the package add itself to a global object is one way to go. Probably there are other approaches for this.

Ps: If the language server always runs in a node context, then I would prefer the require to the global object approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should try and stay away from the global object and dynamic requires. All these concerns should be pushed up the stack so the calling application deals with them directly (and can then choose to do dynamic require vs not depending on what bundling they're using).

With that in mind, should getSchemaLoader be async?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we are working towards allowing this to load behavior based on the platform - so in VSCode that would be via an extension, with node it should
be an async file load not a require, and in the browser we will need to load it differently, probably dynamic import. What it needs is to dynamically decide which platform it's using and to attempt to load the schema loading plugin with the matching mechanism. @Neitsch and I were discussing this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a fix for this now! :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imolorhe as it turns out, webpack should handle the require method just fine, we will shift towards async imports as well, as should electron/etc, as import() is supported in node 9.7+. The other thing is this is on the server side, so this code won't be loaded by webpack.

@Neitsch if you want to update this for the new file resolution utilities we merged a week or two ago (they are in gls-utils now) they should ensure that this dynamic require() line works across environments.

also, is it possible we can use graphql-config to load the schema? looks like it has its own capability for this already. check out the latest 3.0.0 alpha, we will be adopting this soon!

@imolorhe
Copy link
Contributor

Another thing to consider would be if getSchema in the loaderPackage needs to perform asynchronous operations to fetch the schema. In that case, it would be best to have it return a promise from the get-go.

@Neitsch
Copy link
Contributor Author

Neitsch commented Aug 19, 2019

I'll close this PR for now, until I/we have a better story around plugin loading.

@Neitsch Neitsch closed this Aug 19, 2019
@Neitsch
Copy link
Contributor Author

Neitsch commented Sep 18, 2019

After thinking about it a little more and based on @acao 's input on TS plugins, reopening the PR. Will address @imolorhe 's comments next :)

@Neitsch Neitsch reopened this Sep 18, 2019
@acao acao added lsp-server graphql-language-service-server language-services labels Dec 2, 2019
@acao
Copy link
Member

acao commented Dec 2, 2019

@divyenduz what do you think we should do about this? we should be leveraging graphql-config to load the schema correct?

@acao
Copy link
Member

acao commented Dec 13, 2019

@Neitsch i think that in order to accomplish this in the ways that you've indicated facebook and other large scale users might need, it could be that making the graphql-config library itself optional (as some have suggested), though the basic config spec format might still be honored if that's so.

one should of course be able to provide their own configuration management solution in place of graphql-config, and the interface should expect a configuration object that matches up. so graphql spec, but no graphql-config? what do you think?

@ardatan ardatan mentioned this pull request Jan 14, 2020
2 tasks
@acao acao mentioned this pull request Feb 16, 2020
2 tasks
@acao acao added the potential plugin A potential plugin idea for later versions of GraphiQL label Mar 20, 2020
@acao acao closed this Mar 20, 2020
@acao acao added the graphiql label Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphiql lsp-server graphql-language-service-server potential plugin A potential plugin idea for later versions of GraphiQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants