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

feat: optionally provide LSP an instantiated GraphQLConfig #1432

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

zth
Copy link
Contributor

@zth zth commented Mar 17, 2020

This allows supplying a pre-existing graphql-config to the LS. The idea is that external consumers, like a vscode extension, should be able to provide defaults and extensions of the existing config (or generate it programatically all together) in a way that can be supplied directly to the LS.

It's basically #1272 but rebased.

@zth zth mentioned this pull request Mar 17, 2020
@acao
Copy link
Member

acao commented Mar 17, 2020

@zth awesome, thank you! can you allow passing in an instantiated instance to GraphQLLanguageService class as well? For monaco efforts

this would allow us to modify the config object via user input/etc in React, with the current PR

@zth
Copy link
Contributor Author

zth commented Mar 17, 2020

@acao Won't that already work...? GraphQLLanguageService takes a cache, which take a GraphQLConfig when instantiated. Right? Or am I missing how it works?

@acao
Copy link
Member

acao commented Mar 17, 2020

@zth ah yes, it does work that way. we may be migrating GraphQLCache over to interface package anyways

@acao
Copy link
Member

acao commented Mar 17, 2020

@divyenduz what do you think of this?

@zth
Copy link
Contributor Author

zth commented Mar 17, 2020

@acao let me know if there's anything more you'd like me to do here. Would be awesome if this could be included in a release soon, I think this + the last progress you've made means I can drop my fork.

@acao
Copy link
Member

acao commented Mar 17, 2020

@ardatan does this make sense?

@acao
Copy link
Member

acao commented Mar 17, 2020

@zth i think ill merge it anyways, looks like it works the way it should

@divyenduz
Copy link
Contributor

divyenduz commented Mar 17, 2020

Yes, this will be amazing and would ease the transition of VSCode extension from graphql-config v2 to v3.

I was also thinking that LSP can itself have some defaults (like tsconfig has, making this zero-config), basically include all GraphQL files in the workspace etc. What do you think?

@acao
Copy link
Member

acao commented Mar 18, 2020

@divyenduz agreed! so what should those defaults be?

@acao acao changed the title Provide an already instantiated GraphQLConfig feat: optionally provide LSP an instantiated GraphQLConfig Mar 18, 2020
@acao acao merged commit 012db2a into graphql:master Mar 18, 2020
@acao
Copy link
Member

acao commented Mar 18, 2020

thanks so much @zth ! gonna kick out a release as soon as i merge and test the TS/TSX branch

@divyenduz
Copy link
Contributor

@divyenduz agreed! so what should those defaults be?

That's a great question. Maybe on the lines of:

schema: 'src/schema.graphql'
documents: '**/*.graphql'

OR

schema: '**/*.graphql'
documents: '**/*.graphql'

Relying on automatic filtering of documents vs definitions in graphql-config.

There is a challenge though, graphql-js doesn't like type collisions IIRC and if the project is a monorepo with multiple GraphQL servers, this default configuration will most likely crash the language server.

To support a generic configuration like this. We will have to make the language server tolerant of duplicate types.

@acao
Copy link
Member

acao commented Mar 19, 2020

@divyenduz great suggestions! personally, I think the best option would be the first - to default to the single source schema approach, and allow the user to configure in graphql-config, vscode, etc of choice.

in the case of multiple schemas, the user can list multiple schema inputs for each project seperately, but then that way each project would define an entire schema without type collisions? is that something that will be an issue for users?

@zth
Copy link
Contributor Author

zth commented Mar 19, 2020

I think that looks good too! Just one nit - my impression is that the common place for the schema file is in the root, not the src folder. At least that's my experience. Is it common to put it in src?

@kamilkisiela
Copy link

@divyenduz In recent alpha, I added a support for legacy config. You have the same API like loadConfig() and then config.getSchema() but under the hood it checks if provided config has a syntax of v3 or legacy one.

@acao
Copy link
Member

acao commented Mar 25, 2020

@kamilkisiela good to know!

we are using graphql-config@alpha.23 for reference

things are looking great, i think i've almost got it working in web workers and the browser in a webpack setup?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants