Allow async initialization of data sources

[lostfictions]

This tiny PR allows for async initialization of data sources, for use by users extending DataSource or RESTDataSource and implementing an override to initialize().

Here's a real-world use-case: the B2 API requires an initial authorization call to obtain data for subsequent API calls (including a short-lived auth token, an alternate base API url, and potentially other data like bucket IDs that are required in further calls).

Currently, there's not really a great place for this kind of initialization to happen. At first I tried overriding get() to add lazy initialization before allowing the base class to handle the call. That solves my issue where the base URL might not yet be defined when trying to call a rest data source method, but it fails for a method like this:

  listFileNames(data?: {
    startFileName?: string;
    maxFileCount?: number;
    prefix?: string;
    delimiter?: string;
  }) {
    return this.get("/b2_list_file_names", {
      ...data,
      bucketId: this.bucketId
    });
  }

In this scenario, this.bucketId is also provided during the initial API call, so if initialization hasn't happened the get() override will still receive undefined as the value for bucketId.

My current solution is instead to close over all calls to methods like get instead of overriding them:

 getUploadUrl() {
    return this.ensureInit(() =>
      this.get("/b2_get_upload_url", {
        bucketId: this.bucketId
      })
    );
  }

...which solves the issue, but is kludgey and adds a lot of boilerplate. Allowing async initialization would make this scenario much simpler, and should be an unobtrusive add. (Even my change to the return type of DataSource.initialize is optional and just suppresses a hint in VSCode that await won't have any effect.)

[apollo-cla]

@lostfictions: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[trevor-scheer]

Hey @lostfictions, thanks for the contribution and wonderful explanation! This seems like a reasonable change to me 🙂 how would you feel about adding a couple tests to ensure existing behavior and the behavior you're introducing?

[lostfictions]

Okay! Not super familiar with the codebase generally or requestPipeline specifically so I didn't dig into it for unit testing, but I did write a few integration tests that spin up an instance of ApolloServerBase to verify that datasources are passed to resolvers, properly initialized regardless of whether they're sync/async, etc.

Let me know if that seems okay!

[trevor-scheer]

The additional tests are great, thanks for taking the time to add those 🙂 I've recommended one small change then this should be good to go. Let me know if you have any questions!

If you don't mind, please add a CHANGELOG entry to the vNEXT section as well. Thanks again!

[lostfictions]

Thanks for the feedback! I realized today that the way that test was written was just a muddled attempt to check against race conditions -- what we actually want to assert is just some kind of ordering of calls, so I've rewritten it as such. Hope that looks better!

(And fwiw I was aware of Jest timer mocking, but for some reason thought that using it might diminish the ability to check for the race condition if there's costly implementation-level async work happening during query execution... but that doesn't really make sense. On further reflection I suspect it's maybe an antipattern to create new mocked timers in tests -- I imagine Jest's timer mocking is really for mocking timers in implementations, and if you're using it while writing tests you're probably actually just trying to assert some kind of ordering, like I was. I did leave one setTimeout(..., 0) in there to ensure there's at least an async function that lands on the real task queue rather than just the microtask queue like Promise.resolve(), but this is entirely paranoia :)

[lostfictions]

It belatedly occurred to me that this was also awaiting each initializer serially (for(...) { await ... }) rather than in parallel. Fixed! (There's an ESLint rule for this, but it doesn't look like this repo uses ESLint...?)

[lostfictions]

@trevor-scheer small ping on this! would love to see it merged if it looks okay, rather than maintaining my own fork :)

[trevor-scheer]

@lostfictions great catch, and thank you for the additional thought and care you put into this 😄
Everything here LGTM! I'll speak with @abernix about landing and releasing this change on Monday.

[abernix]

LGTM! Thanks so much for taking the time to clearly explain the use-case, make changes in a backward-compatible manner and put together a clean PR!