feat: allow stringifyResult to return a Promise<string>

[favna]

Previously stringifyResult had to be synchronous, despite only ever being called from the async context of runHttpQuery. This is an issue when the custom stringification function provided by the user through the constructor has to be async, for example when working with very large POJOs one could use json-stream-stringify because JSON.stringify would throw a RangeError with Invalid string length, the implementation using this library would be:

    stringifyResult: async (value) => {
      const stringifyStream = new JsonStreamStringify(value);
      let stringified = '';

      for await (const chunk of stringifyStream) {
        stringified += chunk;
      }

      return `${stringified}\n`;
    }

As can be seen here, this requires an async context within the callback function. @apollo/server can easily support this by awaiting the result of internals.stringifyResult.

Furthermore I also changed errorResponse to use the same internals.stringifyResult for consistency of how the JSON that will be sent to the requester is stringified.

[apollo-cla]

@favna: 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/

[netlify]

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	d7bf521

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d7bf521:

Sandbox	Source
Apollo Server Typescript	Configuration
Apollo Server	Configuration

[trevor-scheer]

Overall the change looks good to me. I agree with supporting the async use case here, and we probably should've made it async in the first place. I also agree not updating the errorResponse function was an oversight in the original PR, so I'm comfortable with treating this as a bugfix rather than a breaking change in how stringifyResult works.

I have a couple requests:

• Please mention the bugfix / change in behavior of errorResponse in the changeset. It would be helpful to note who would be affected and what the expected behavior change is (i.e. "Users who implement the stringifyResult hook can now expect their error responses to be formatted with the hook as well. Please take care when updating to this version to ensure this is desired behavior, or implement the desired behavior accordingly in your stringifyResult hook.")
• Can you please add a test to exercise the new stringifyResult + error case?

This one's a bit more theoretical and wordy. I think the change to making errorResponse async requires the addition of await at all callsites. While we don't have any tests that catch the issue I'm imagining, I do think there's a change in behavior here if we don't await them. Right now those callsites are wrapped in try/catch blocks. If we return the promise (as we're doing now), we "escape" the try/catch, leaving a promise rejection to be handled elsewhere. If we await those calls, the catch block will execute resulting in no behavior change from what exists now.

I found a blog posts that articulates precisely what I was thinking.
https://jakearchibald.com/2017/await-vs-return-vs-return-await/#return-awaiting

[favna]

Addressed your review comments @trevor-scheer. In regards to the return await matter for which you linked the Jake Archibald blogpost, luckily I'm well aware of how the JS event loop handles that kinda stuff so I immediately knew what you were referring to :) (bless Jake Archibald btw, absolute genius). I believe I have covered your requests now.

[trevor-scheer]

Sorry about the cspell, I would've also accepted adding behaviour to the dictionary. Honestly surprised it didn't like that in the first place. Thanks for making those changes, this lgtm. I appreciate the PR!

[favna]

Haha, no problem at all on the cspell. I actually chuckled to see CircleCI fail there. No need to apologise. Thanks for the approval!

[trevor-scheer]

Apologise...funny guy 😜

[favna]

@trevor-scheer Hi there, I would like to ask you about the release schedule for Apollo Server. Specifically, when will 4.10.0 be released so I can use it in my project? The contributing.md file mentions a file roadmap.md but that file no longer exists so I have no information on this topic.

[glasser]

@favna Most of Apollo was on vacation last week, but Trevor has now released this in Apollo Server 4.10.0!

[favna]

@glasser Totally understandable, I hope you guys had nice holidays and best wishes for the new year. I was pinged by the GitHub bot yesterday so I noticed and I have since updated my application. Many thanks for the Apollo project!