Online schema support #215
Conversation
lostplan
force-pushed
the
online_schema
branch
from
837c860 to
f7528c6
Compare
|
I'd really appreciate some feedback on this. Any maintainers out there willing to take a look? @asiandrummer would you mind? |
packages/server/src/GraphQLCache.js
Outdated
| if (endpointInfo) { | ||
| schemaCacheKey = `${endpointInfo.endpointName}:${projectName}`; | ||
|
|
||
| // Maybe use cache |
There was a problem hiding this comment.
One thing that would be cool to do later is to read the schema info from the endpoint every time to invalidate the cache. When it was a physical .graphql file, the language service was able to detect the changes to the schema file as one of the extensions and extend the schema with it as well as invalidating it. I'd say it's pretty important to get the fresh state of the schema as long as we're editing the graphql files that depend on this schema.
A couple things about this:
- definitely not required to merge this PR
- it shouldn't probably refetch for every updates that triggers this method
There was a problem hiding this comment.
Agreed. I think periodic cache invalidation is probably the way to go.
| @@ -107,7 +107,11 @@ export class GraphQLLanguageService { | |||
| ]; | |||
| } | |||
|
|
|||
| if (!schemaPath) { | |||
| const schema = await this._graphQLCache | |||
There was a problem hiding this comment.
Would it be possible not to await for the schema to be created here and below? The reason why we sanity-check the path (and the endpoint in addition according to this PR) is not to spend any unnecessary time to compute the schema if it's not necessary. This usually isn't a problem unless there's a validation error in the schema creation process, in which this will execute and fail every time.
There was a problem hiding this comment.
I think the best way to prevent thrashing is to "cache" both successful schemas and failures (both from disk and network) and disallow re-fetching until expiry. In which case we would keep await getSchema but trust that it will fail fast. However this feels beyond the scope of the current change – would you be happy to merge this change without it?
There was a problem hiding this comment.
I think we should merge with the behavior as is and file a ticket to address this failure mode next. Existing users won’t be impacted since there is currently no online schema support anyway. If they have have broken disk schema this await will fail fast.
| const schemaPath = projectConfig.schemaPath; | ||
| const schemaCacheKey = `${schemaPath}:${projectName}`; | ||
| const endpointInfo = this._getDefaultEndpoint(projectConfig); |
There was a problem hiding this comment.
nit:
const {endpointName, endpoint} = this._getDefaultEndpoint(projectConfig);There was a problem hiding this comment.
Oh, it looks like _getDefaultEndpoint can return something that is falsy so destructuring here could possibly throw an error which is why I think it's handled below.
| describe('getSchema', () => { | ||
| it('generates the schema correctly for the test app config', async () => { | ||
| const schema = await cache.getSchema('testWithSchema'); | ||
| expect(schema instanceof GraphQLSchema).to.equal(true); | ||
| }); | ||
|
|
||
| it('does not generate a schema without a schema path', async () => { | ||
| it('generates the schema correctly from endpoint', async () => { |
It is optional
- pass directly through to extendschema to avoid duplicate logic - use endpoint name instead of url (as this may not be unique)
| const schemaPath = projectConfig.schemaPath; | ||
| const schemaCacheKey = `${schemaPath}:${projectName}`; | ||
| const endpointInfo = this._getDefaultEndpoint(projectConfig); |
There was a problem hiding this comment.
Oh, it looks like _getDefaultEndpoint can return something that is falsy so destructuring here could possibly throw an error which is why I think it's handled below.
Use
graphql-config's endpoints extension to read schema directly from the graphql API endpoint. If no endpoint is defined, or the network request fails, it will fall back to reading the schema fromschemaPath(as before).This change includes some tweaks to how the schema cache key is calculated to allow for the new schema source.
N.b. If multiple endpoints are defined,
graphql-configwill readprocess.env. GRAPHQL_CONFIG_ENDPOINT_NAMEto determine which should be used (source).Fixes #213