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

Feature: allow dataSources creator function to be async #4222

Closed

Conversation

rintaun
Copy link

@rintaun rintaun commented Jun 8, 2020

#3639 changed initializeDataSources to be an async function, so it is a small step to allow the supplied dataSources creator function to also be async. This helps simplify things when the creation of the dataSources object itself (rather than each individual entry) relies on some asynchronous process, e.g. connecting to a database or getting a connection from a pool.

Right now, we are using a factory for the dataSources creator function that waits on the connection pool, but it would be nice to get rid of that extra step.

Simplified example:

const dataSources = async () => {
  const pool = await createPool()
  return () => ({
    // ...data sources...
  })
}

new ApolloServer({
  dataSources: await dataSources(),
})

After this change, this could be represented as:

const dataSources = async () => {
  const pool = await createPool()
  return {
    // ...data sources...
  }
}

new ApolloServer({
  dataSources,
})

Note: the second approach also makes it possible to perform some asynchronous process on every request rather than only once at startup as in the first example.

@rintaun
Copy link
Author

rintaun commented Jun 8, 2020

Not sure what the test failures in apollo-gateway are about... I'm not seeing those locally.

@abernix abernix closed this Jun 24, 2020
@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:01
@jldillman
Copy link

@rintaun This feature would be awesome. Do we know if there's a reason this hasn't been approved yet?

@rintaun
Copy link
Author

rintaun commented Nov 8, 2020

@jldillman no idea! probably because I put it up randomly without any discussion anywhere 😂

Personally, I'm not doing anything that would require this feature anymore, though that might change if this were merged.

@iquabius
Copy link

Is something holding this back? It's just a single word of behavior code change: await. The rest are types and testing.

In case anyone needs I used patch-package to apply the changes in this PR.

Click to see the apollo-server-core+2.23.0.patch file
diff --git a/node_modules/apollo-server-core/dist/graphqlOptions.d.ts b/node_modules/apollo-server-core/dist/graphqlOptions.d.ts
index 6a9b167..a6be275 100644
--- a/node_modules/apollo-server-core/dist/graphqlOptions.d.ts
+++ b/node_modules/apollo-server-core/dist/graphqlOptions.d.ts
@@ -21,7 +21,7 @@ export interface GraphQLServerOptions<TContext = Record<string, any>, TRootValue
     tracing?: boolean;
     cacheControl?: CacheControlExtensionOptions;
     extensions?: Array<() => GraphQLExtension>;
-    dataSources?: () => DataSources<TContext>;
+    dataSources?: () => DataSources<TContext> | Promise<DataSources<TContext>>;
     cache?: KeyValueCache;
     persistedQueries?: PersistedQueryOptions;
     plugins?: ApolloServerPlugin[];
diff --git a/node_modules/apollo-server-core/dist/requestPipeline.d.ts b/node_modules/apollo-server-core/dist/requestPipeline.d.ts
index 9660907..1c707bb 100644
--- a/node_modules/apollo-server-core/dist/requestPipeline.d.ts
+++ b/node_modules/apollo-server-core/dist/requestPipeline.d.ts
@@ -14,7 +14,7 @@ export interface GraphQLRequestPipelineConfig<TContext> {
     validationRules?: ValidationRule[];
     executor?: GraphQLExecutor;
     fieldResolver?: GraphQLFieldResolver<any, TContext>;
-    dataSources?: () => DataSources<TContext>;
+    dataSources?: () => DataSources<TContext> | Promise<DataSources<TContext>>;
     extensions?: Array<() => GraphQLExtension>;
     persistedQueries?: PersistedQueryOptions;
     formatError?: (error: GraphQLError) => GraphQLFormattedError;
diff --git a/node_modules/apollo-server-core/dist/requestPipeline.js b/node_modules/apollo-server-core/dist/requestPipeline.js
index 50f5741..a2e1628 100644
--- a/node_modules/apollo-server-core/dist/requestPipeline.js
+++ b/node_modules/apollo-server-core/dist/requestPipeline.js
@@ -353,7 +353,7 @@ function processGraphQLRequest(config, requestContext) {
             return __awaiter(this, void 0, void 0, function* () {
                 if (config.dataSources) {
                     const context = requestContext.context;
-                    const dataSources = config.dataSources();
+                    const dataSources = yield config.dataSources();
                     const initializers = [];
                     for (const dataSource of Object.values(dataSources)) {
                         if (dataSource.initialize) {
diff --git a/node_modules/apollo-server-core/src/graphqlOptions.ts b/node_modules/apollo-server-core/src/graphqlOptions.ts
index 9af8e6b..122b0a1 100644
--- a/node_modules/apollo-server-core/src/graphqlOptions.ts
+++ b/node_modules/apollo-server-core/src/graphqlOptions.ts
@@ -58,7 +58,7 @@ export interface GraphQLServerOptions<
   tracing?: boolean;
   cacheControl?: CacheControlExtensionOptions;
   extensions?: Array<() => GraphQLExtension>;
-  dataSources?: () => DataSources<TContext>;
+  dataSources?: () => DataSources<TContext> | Promise<DataSources<TContext>>;
   cache?: KeyValueCache;
   persistedQueries?: PersistedQueryOptions;
   plugins?: ApolloServerPlugin[];
diff --git a/node_modules/apollo-server-core/src/requestPipeline.ts b/node_modules/apollo-server-core/src/requestPipeline.ts
index d448b4d..1e2f038 100644
--- a/node_modules/apollo-server-core/src/requestPipeline.ts
+++ b/node_modules/apollo-server-core/src/requestPipeline.ts
@@ -92,7 +92,7 @@ export interface GraphQLRequestPipelineConfig<TContext> {
   executor?: GraphQLExecutor;
   fieldResolver?: GraphQLFieldResolver<any, TContext>;
 
-  dataSources?: () => DataSources<TContext>;
+  dataSources?: () => DataSources<TContext> | Promise<DataSources<TContext>>;
 
   extensions?: Array<() => GraphQLExtension>;
   persistedQueries?: PersistedQueryOptions;
@@ -740,7 +740,7 @@ export async function processGraphQLRequest<TContext>(
     if (config.dataSources) {
       const context = requestContext.context;
 
-      const dataSources = config.dataSources();
+      const dataSources = await config.dataSources();
 
       const initializers: any[] = [];
       for (const dataSource of Object.values(dataSources)) {

@glasser
Copy link
Member

glasser commented Aug 2, 2021

Practically speaking, the data sources subproject is currently unmaintained, in the sense that the current Apollo Server team members have not yet fully "on-boarded" ourselves to that subproject (which is a bunch of code almost entirely unrelated to the rest of Apollo Server) since we revived the Apollo Server project earlier this year.

(For what it's worth, if/when I do find the time to onboard myself onto data sources, my first project will probably be to deprecate the top-level dataSources argument. Data sources were added to AS around the same time plugins were, and in retrospect they could easily just be provided via a built-in plugin instead of via their own top-level function. Making this change would make solving this particular issue easy, because the plugin hooks are already almost all async. See #4950)

@glasser
Copy link
Member

glasser commented Oct 7, 2022

Similar to what I mentioned in my previous comment, we've removed the dataSources option in Apollo Server 4 (currently a Release Candidate). dataSources is just a special function for initializing part of context and it's simpler to just initialize your entire context value in a single function. (And the context function is, in fact, async.) Sorry for the delay, but I think this does resolve the same issue!

@glasser glasser closed this Oct 7, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants