-
Notifications
You must be signed in to change notification settings - Fork 94
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
GraphQL Configuration Protocol #20
Comments
@asiandrummer very clean! I'd like it.
- env?: EnvironmentVariable,
+ env?: GraphQLConfigurationEnvs,
- extension?: Any,
+ extension?: GraphQLConfigurationExtension,
+ export type GraphQLConfigurationExtension = {
+ [toolName: string]: any,
+ };
|
@nodkz edited with suggestions! For 2., I added |
@asiandrummer |
@nodkz I'm also terrible at naming things ;) I'm open to other suggestions please! |
Another addition that may come later is a glob pattern for locating directories:
|
@nodkz @schickling I'm going to write something up to be included in graphql.org page soon - I'll cc both of you to the PR/issue in that repo when I create one! |
Progress update: I haven't spent much time to work on this until today - but I still wanted to communicate how I thought the further process should be.
I'm in progress of completing (1), and will open a PR once done and include stakeholders. The whole progress has been somewhat stagnant and I apologize for the delay, and I'll make sure to spend time to work on this consistently. For that, I have some sort of deadline in my mind (by the end of March) to finish this project. |
Thanks a lot for that great progress update. We're happy to tackle (2). Should be feasible within the next week. I'll ping you in Slack to coordinate on this. |
I'm trying to wrap my head around what the inputs/outputs of something utilizing this config is supposed to be. I think it's
One of the problems I've run into, is that I need to parse graphql from a lot of different sources: JS files, .graphql files, schema files, etc., and the AST coming out of each individual source is often not able to be validated until combined with all the other AST sources. As such, I think GraphQLConfiguration should support combining files that need to be parsed in entirely different ways, apply a schema to the parsed result, so you can get a validated schema + definitions. Some tweaks I'd propose making:
Additionally, it seems like env should always live outside, not inside, the configuration. Each configuration describes a single environment (whether that's production or development or separate apps). When parsing raw files, you can choose to use multiple configurations, and switch to the relevant configuration (you might have a map of map of configurations, if you have 3 different apps to configure, each with their own environments. But that should be fine). |
Wouldn't we be still able to use
The env variables could certainly be used in that way, but I could see how this could be useful if, during the build/compile time, the tool used wants to choose a different environment. I prefer to separate those using app configs though |
cc @nodkz @schickling @mjmahone @wincent @martijnwalraven - including everyone who participated in the discussions for a final review. I've taken some time to get more feedbacks internally from Facebook, iterate on the writing a few times, and drafted up a final version of what I'll create an issue with Monday to graphql.org. I'd appreciate if you could take a look - we can also continue the discussion in the issue I'll be creating. The most notable changes are the addition of For the reason above, I've moved Lastly, for the starters, I'll be submitting a few PRs to make GraphQL ConfigurationThere are many ways to configure your application to use GraphQL, and while it is often enough to specify configuration options directly in your application code, maintaining and understanding the hard-coded configuration options may become a challenge as the scale grows. We recommend configuring your application with a GraphQL Configuration FormatAfter inspecting many GraphQL applications for use cases and communicating with many external contributors, we recommend configuring your GraphQL application with a below format:
Use CasesUsually, for a simple GraphQL applications, the only required configuration is a way to fetch a GraphQL schema:
Although it is possible for the configuration file to have more than one way to fetch the schema, a GraphQL application consuming this configuration should determine how to work with provided configuration properties. For example, while one application may arbitrarily choose to have
For multiple apps with isolated directories, there are mainly two ways to set up the configuration. Each app can either use the
Or each can app have a separate
Default Configuration Properties
Consider the below GraphQL configuration:
Since projectA and projectB share the same schema file, we can push the
|
@asiandrummer sorry i'm on vacation now till 18 April. Btw, for me |
Hey @asiandrummer, @nodkz, @schickling, @IvanGoncharov and I started working on the initial implementation of this proposal on the branch of this repo. After reviewing this and previous threads we have a few suggestions/proposals. First of all, I want to emphisize an important use case: the ability to execute GraphQL queries from inside the editor. I don't see so much value in writing GraphQL queries inside editor if you can't execute them from inside. In such case, users will just write queries in GraphiQL and copy-paste them to editors. And since GraphiQL already has validation and auto-suggestions, there is no need for those in editors. E.g. GraphQL Intelliji plugin supports this as a feature:
So I think this is really important use-case which we should support in graphql-config core.
The example of utilizing this property is GraphQL Intelliji plugin which has its own config format at the moment. This plugin has built-in support for executing GraphQL queries and there is config property Also, we think
There is an important use case: writing project against public APIs like GitHub, Yelp, Shopify. In such case, users can't be already authenticated because of obvious reasons.
Summing up, we think it makes sense to move server url and headers to core config. In Flow-like it may look like:
What do you think? About the idea suggested by @wincent:
This is a great idea but it introduces difficulties. If merge configs from different levels we loose info about config location and are not able to resolve relative paths from config. So we suggest to not merge configs, at least in the initial version. |
Together with @RomanGotsiy we implemented @asiandrummer proposal on the config-protocol branch. We borrowed a lot of ideas from graphql-language-service-config since it provides everything you need for GraphQL support inside IDEs by having per-file API. However, not every tool works on per file basis, e.g. mocking your GraphQL API or checking schema for breaking changes. So we tried to accommodate both cases by splitting functionality into the two classes: class GraphQLConfig {
public config: GraphQLConfigData
public configPath: string
constructor(
public rootPath: string = process.cwd()
)
getProjectConfig(projectName: string): GraphQLProjectConfig
getConfigForFile(filePath: string): GraphQLProjectConfig | null
getProjects(): { [name: string]: GraphQLProjectConfig }
}
class GraphQLProjectConfig {
public config: GraphQLProjectConfigData
public configPath: string
constructor(
path: string = process.cwd(),
public projectName: string = process.env.GRAPHQL_PROJECT,
configData?: GraphQLConfigData // for cases when data is already parsed
)
resolveConfigPath(relativePath: string): string
resolveSchema(env?: string): Promise<GraphQLSchema>
resolveIntrospection(env?: string): Promise<any>
resolveSchemaIDL(env?: string): Promise<string>
getConfig(envName: string = process.env.GRAPHQL_ENV): GraphQLResolvedConfigData
getEnvs(): { [env: string]: GraphQLResolvedConfigData }
includesFile(filePath: string): boolean
} The simplest example would be: const config = new GraphQLProjectConfig('./', 'OptionalProjectName');
const schema = config.resolveSchema('OptionalEnvName'); In more complicated scenarios you can get per-file schema like that: const config = new GraphQLConfig('./');
const perFileConfig = config.getConfigForFile('./someQuery.js');
const perFileSchema = perFileConfig.resolveSchema('OptionalEnvName'); @asiandrummer, @nodkz, @schickling, |
@IvanGoncharov @RomanGotsiy first of all thanks for taking point to implement the details! Please tag me in the PR once you make one and I'll make sure I review it. But before anything I've read through your proposals and wanted to ask some questions below. @IvanGoncharov from your proposed API, I don't think
I disagree - although being able to execute queries inside the editor is a cool feature addition to each IDEs, it doesn't devalue the ability to run the language service in each IDEs. For the simplest example, the query validation would serve as the first line of defense in passing your queries as usable ones. Go-to definition support is almost completely lacking in GraphiQL, and is a different use case from executing queries.
I agree that's a clearer use case - however I'd be more comfortable to add it in the actual spec doc (if we get to have one later) after seeing some more use cases/needs. The main concern I have is that the
Sorry if I'm misunderstanding, but could you elaborate here? How could you work around storing a sensitive information in a JSON file by using env variables, which would be stored in the same JSON file?
I'm not sure why more than a server URL is needed to configure the GraphQL server, even if you're using the server information from the config to do language-agnostic stuffs (such as build/compile). Considering the mentioned use case is being able to authenticate and fetch information easily, I can see why putting the server configuration in the JSON file (graphqlconfig) is more convenient - if one chooses to do so, My point is that it feels like the detailed server information/options in GraphQL configuration file won't really be a general configuration option (and personally a discouraged approach). I would like to understand more about the needs for such an info since I might be missing something huge here. |
An addition I'm hoping to add is a
At Facebook, I'd like to have this for a script to uniquely identify individual projects that exist throughout our codebase. While in my case, I require each name to be unique, that doesn't have to be a part of the spec. This can simply by a useful tool for existing tools out there to use the name value to refer to the project when outputting debug messages. (within https://github.com/graphcool/graphql-config/blob/config-protocol/src/utils.ts#L56 , we could use this for nice error messages 😄 ) For multi-project, this might not be as useful since a projectName is already defined as keys, but might still be useful for human friendly names? For other scripts and tools that require a way to uniquely identify projects (like I do), this could serve that purpose 😃 |
@asiandrummer We made implementation according to your proposal so we fully support
A @RomanGotsiy example was about OS environment variables not environment variable inside config file. Our idea is to allow you to specify header both as a string or as a reference to OS environment variable. {
"headers": {
"User-Agent": "Build script",
"Authorization": {
"envVariable": "GITHUB_AUTHORIZATION"
}
}
} In above sample
At Facebook, you work only with your internal GraphQL APIs which are accessed behind a firewall so you don’t need any And this is not theoretical. BTW. I’ve just watched Robert Zhu talk at the OpenStack conference and here is a relevant slide from it: @pranaygp Thank you for taking a time to review our implementation.
Can the file path to config file be used to uniquely identify config inside error messages? |
Cool - sorry for the confusion. I missed that one.
On the contrary, we do have some internal usages that require access tokens to reach GraphQL endpoint. Just like the slide you've included, the setup to use the endpoint is done within the code, and the necessary configuration is included close to the code that it's used to execute the http request.
I'm fully in agreement with you that most of the times a proper authentication is needed to access third-party GraphQL endpoints, including the data points you've mentioned. What I want to reiterate is that GraphQL configuration may not be the right place for them. I want to accentuate the need for the separation of concerns - to summarize, the core of GraphQL configuration should only achieve a few things: defining the location to get GraphQL schema, and defining where the relevant files are to GraphQL artifacts. In addition, |
Also, thanks to everyone's participations, I feel like we're ready to move this forward and actually create a PR to add this as a protocol/spec document. I'll get that done soon - we can continue these valuable conversations over there. |
@asiandrummer Just to be sure we are on the same page here, where should Because in my and @RomanGotsiy comments we assumed that {
// How to resolve this is up to the application
"schemaPath": "./schema.graphql",
"schemaUrl": "http://my-app.com/schema"
} |
Hi @IvanGoncharov, apologies for the radio silence on my end these days. I was meeting with @asiandrummer (and other amazing folks from Facebook) this week and we discussed the next steps for We concluded the following:
To discuss:
ExamplesTo wrap up, here are a few examples using the current format proposal. Minimal{
"schemaPath": "./schema.graphql",
"extension": {
"endpoint": "https://api.graph.cool/instagram"
}
} Multi-project{
"development": {
"schemaPath": "./dev.graphql",
"extension": {
"http-endpoint": "http://localhost:3000/graphql"
}
},
"production": {
"schemaPath": "./prod.graphql",
"extension": {
"http-endpoint": "https://api.graph.cool/instagram"
}
}
} |
We have some questions about
You previously mentioned that
But there is an issue with that. Assuming use case 2 and 3 and e.g. the following config: {
"production": {
"schemaPath": "./prod.graphql",
},
"dev": {
"schemaPath": "./dev.graphql",
}
} With the current language services API: getAppConfigNameByFilePath(filePath: Uri): ?string { how would you decide which schema to use? As far as I understand the current use case is editor passes path to the file and that's it. One option would be changing
What do you think? |
@IvanGoncharov allow me to reply to your comment in regards to For all 3 cases above, if you cannot associate one project with one schema, with a list of
I chatted briefly with @schickling about supporting features in Would this be okay? |
@asiandrummer Good night 😄 No need to reply. It's now clear from @schickling comment.
Yes, it would be a good starting point. |
@asiandrummer together with @RomanGotsiy and @schickling we've discussed @nodkz Presented another use case for
That's why we decided to remove
We also made a few small changes to your proposal as you can see in this diff.
What do you think? |
@IvanGoncharov I prefer
As a working example of complex {
...defaultOptions,
env: {
production: {
...mergableOptionsWithDefaultForProduction,
},
development: {
...mergableOptionsWithDefaultForDevelopment,
},
staging: {
...mergableOptionsWithDefaultForStaging,
},
[anyOtherEnvName: string]: { ...opts },
// projects can be setupped in following manner
production_projectA: { ... },
production_projectB: { ... },
},
}; PS. @asiandrummer how do you setup |
@nodkz Biggest problem I see with environment variables is that they are CLI oriented. One of the main use cases for And this is where configuration through environment variables create a few problems:
So by using |
I'm using Atom by clicking an icon and not by running CLI command. Atom makes From babel docs:
So for IDE by default it tries to get merge( {
...defaultOpts,
development: {
...devOpts,
}
} if And yep, it is cool feature to run editor with ENV variable for obtaining production/staging transpilling, checks and connections. I have not seen in any
How I understood |
@asiandrummer I assume you're understanding Facebook's use case best, can you provide some background why this is desirable? Having multiple "smaller" |
@nodkz @IvanGoncharov @schickling Let me come back in a couple hours and answer this if that's okay with you guys - sorry! Things are a bit crazy on my side right now. |
IMO I'd prefer With the discussion around Babel configuration files, I actually think babelrc and graphqlconfig serve two almost completely different use cases, which I can elaborate below. But for this reason I don't think we can relate the usages and purposes for
Within Facebook we include I would argue that configurations for Babel and GraphQL tools/apps have vastly different purposes. Babel is a tool that accomplishes a specific set of goals, thus the goal of a configuration file (
IMO @schickling I'm ready to move forward with the spec + some additions @IvanGoncharov made. Let me PR this out and we can continue this conversation in that PR. |
Closing as 9e7aef7 has been committed. I'll add |
GraphQL Configuration protocol
In a nutshell, the simplest format is:
Note that I'd like to discourage the recursive pattern in using the
EnvironmentalVariable
- for example:Now the reasoning for it: let's start with a most simple use case - one app/one schema.
Single-app/single-schema
Usually a single app requires a schema path/url and an env variable to facilitate the build and deployment:
Multiple-apps/isolated directories
For multiple apps with isolated directories, I like @wincent's idea about a common-default GraphQL configurations, but in spirit of keeping things simple at first, I'd like to propose an one-config-per-directory approach as our first draft. I think there's an edge case where we'd have multiple parent GraphQL configurations to consider:
This can become a discussion of its own, which we can have separately ;)
Custom validation rules
You may want to run custom validation rules before building GraphQL apps/codegen/etc -
customValidationRules
is useful for that.Reserved namespaces
@wincent made a suggestion to provide a way to include any other configurations if you desire. This namespace will contain an app/prod/organization/team-specific GraphQL configurations, and it will be treated as optional settings you'd like to use for your purpose.
What is this repo, then?
I think this repo should serve two roles in a broader sense:
1. Maintain this protocol and become a town hall for discussions/improvements for this protocol.
2. Provide reference implementations of helper methods to lubricate adoption for this protocol.
An example of the reference implementation of the helper method: we mentioned a way to find this GraphQL configuration is to walk up a directory until it finds one. This basically can be implemented as such:
As there are several use cases, we can tackle each of them and present a way to use this configuration programmatically. I can begin by identifying/suggesting some of them below:
GraphQLSchema
object using schema definitions in .jsAction items
There are many things to do! But when we successfully deliver this, I believe we can get to the point where we have a shared GraphQL configuration that everyone agrees on, with commonly-used techniques and best practices to configure your GraphQL environment.
graphql.org
Lastly, although we've begun this proposal and made a bunch of suggestions, I'd love to have a collaborative effort in pushing this forward. It'll be amazing to see us working together to improve and deliver this proposal to the GraphQL community.
Please post your thoughts/concerns/questions! Also if you'd like to be responsible for one of the actions items above, don't be hesitate to do so :D
The text was updated successfully, but these errors were encountered: