Scaffold out new transport mechanisms #3576

abernix · 2019-12-02T21:08:42Z

Updated since this PR body was originally written!

This kicks off the beginnings the transport abstraction which I originally proposed in #3184. This builds on the new execution functionality @trevor-scheer added in #3423 ~~, but doesn't yet consider the new "lift-off" work by @queerviolet in #3571.~~ (We will not consider that further, for the time-being!)

I've intentionally marked some tests that still need to be written with it.todos, while other newly added tests simultaneously tick boxes for coverage needs I'd previously noted in #3303 as lacking. (🎉) (Update: There are only a small number of it.todo tests now — 6 perhaps? — as most have been completed.).

Some "todo"-ish tests within this PR are marked as it.skips because I'm unsatisfied with the completeness of some more broad handler functionality. (In fact, I backed out a subset of the handler functionality — which didn't get as much attention as I'd hoped to give it — entirely because I want to take another shot at it with recently informed perspective. I'm happy letting that surface in a later PR and turning our attention to some more pressing design decisions we have to make.) Update: No tests are it.skip'd anymore.

That's not to say that this handler and transport doesn't work and can't land ~~, but I really want to spend some more time with the tests in particular, which I'm certain will reveal some fires. (Because, tests!)~~ Update: Oh, you better believe the tests found some problems!

For now, the handler can be used as:

import { ApolloServer, gql, httpHandler } from '@apollo/server';
import express from 'express';
const app = express();

const server = new ApolloServer({
  modules: [{
    // More complete SDL is necessary!
    typeDefs: gql`scalar AMoreCompleteSchema`,
    resolvers: {}, // Do supply these!
  }],
});
app.use('/graphql', httpHandler(server.executeOperation));

~~There are a number of very important tests that need to be written~~, and there's ~~some rejiggering to be done to make this play nicely with the ApolloServer constructor (if we want that)~~ and, ~~more importantly, the lift-off work,~~ but ~~it'd be better to get~~ some feedback on the current progress ~~since some of the core pieces are changing quickly~~ please!

TODO (in follow-up PRs)

Confirm if every error response should be and is application/json. Currently, there are some exceptions to this during parsing. These can be easily changed.
Implement a method for creating the transport context. Determine how the transport context relates to the user context (e.g. ApolloServer's context parameter). Question: Need any implementation consider or demonstrate either the use of continuation local storage (e.g. @wry/context, cls-hooked, express-http-context)?
Determine if we should move away from __apolloHttpStatusCode and what the technique should be for adjusting these.
Use a more defensive technique (e.g. throwing specific errors, perhaps à la Error handling from liftoff #3645, within processGraphqlRequest) for recognizing validation and parsing errors accurately.
Return a 405 when a mutation is attempted via GET. This requires participation from processGraphqlRequest.
Explore and think about how extensions present on the request parameters need to be constructed. Currently I believe they initialize graphql-extensions classes.
Breaking change consideration: Status code 207 is now used as a Mixed-response status code, e.g. when an error has occurred within resolver execution.
Think about caches and how they might live and be configured on the transport: e.g. APQ, etc.
What about calling next?

…ant. We started off building off some of the Apollo Server 2.x base, and that is no longer as feasible with our new directions!

… local.

TypeScript was suggesting this, and I believe that it makes sense!

My fault! Not only should this match the file that it tests (transport.ts), but it should also have `.test.ts` in the name for consistency with other test files, but also for a future where we quash the `__test__` directory entirely.

Amongst the changes in this commit: * We now define our own `GraphQLRequest` and `GraphQLResponse`, which do not use the versions in `apollo-server-core` which previously declared an `http` property that is no longer relevant (or desired!) * The `GraphQLResponse` now uses type predicates to narrow the properties to those which "must" be implemented by the spec given the specific conditions. * Uses consistent interface names. * Renames test files to match the files they're testing.

abernix

I won't comment on every TODO, but here are some thoughts / notes-to-self on some of them.

v3/src/transports/http/__tests__/handler.test.ts

v3/src/types/index.ts

abernix · 2019-12-02T21:11:38Z

v3/src/transports/http/transport.ts

+    error.locations.length
+  ) {
+    if (Array.isArray(error.nodes) && error.nodes.length) {
+      return 400; // Validation error


We need to introduce specific types into the stages of the request pipeline to capture these precisely!

Do they have error codes we could use?

Not yet. We previously had ValidationError and ParseError, but those have not re-surfaced in the new machinery yet. cc @trevor-scheer something we'll need.

Definitely! Any reason in particular for the hesitations on using apollo-server-errors? Are we interested in having those live within the v3 directory / @apollo/server package?

Worth noting, we do have a breaking change to make on the ApolloError type, so leaving it all behind is really not a terrible option.

What is that breaking change? I thought we were just going to include code properties on errors, but I think we could still support ApolloError style errors, perhaps?

Should we use #3645 for these errors? Can we make ValidationError implement the new errors in a way where they remain compatible with existing users, e.g., who might be doing instanceof checks against ValidationError and ParsingErrors in their plugin hooks?

Sorry I didn't respond to this sooner!

What is that breaking change? I thought we were just going to include code properties on errors, but I think we could still support ApolloError style errors, perhaps?

Right now, properties set on the extensions object are added as properties to the Error itself. This is old behavior that we chose to preserve but it's not spec-compliant. I wrote some code awhile back that gives us (and users) a migration path away from depending on this behavior with the intention that we'd break support for it in the future.

apollo-server/packages/apollo-server-errors/src/index.ts

Line 40 in faba52c

this[key] = extensions[key];

v3/src/transports/http/handler.ts

abernix · 2019-12-02T21:15:18Z

v3/src/transports/http/__tests__/transport.test.ts

+      ],
+      throwForbiddenError() {
+        // TODO(AS3) What should we do with these?
+        throw new ForbiddenError("Not allowed!");


We should chat about these error classes from apollo-server-errors. They are very much a thing from Apollo Server, but I believe current properties of the response from execution make it difficult to use these to decidedly calculate an HTTP status code for a partial success response, which is an important detail to iron out.

This isn't resolved, but let's nail down the role of #3645 here and consider how we expect like this within resolvers to affect the status code. Considering a @defer'd resolver may not resolve until long after the headers are sent, I don't think it's necessarily practical to change the status code based these existing apollo-server-errors classes we've previously provided.

queerviolet

Super happy to see us pulling apart execution and transport! I have some thoughts on how to connect those two layers, particularly in the face of changing schemas.

v3/src/execution/index.ts

v3/src/transports/http/__tests__/transport.test.ts

queerviolet · 2019-12-03T10:05:04Z

v3/src/transports/http/handler.ts

+ * a `RequestHandler` that can be used with Node.js' `http.createServer`, or
+ * Express' `app.use`.
+ *
+ * @param apollo An instance of `ApolloServer`


this is a lie

Good catch, it is! Though the previous implementation I had did leverage an instance of ApolloServer, I changed that to align with the expectations of the new processGraphqlRequest from #3428

I'll adjust this, but speaking to the maintainability of these TSDoc notations though: This mismatch marks, in my experience on this new code-base, maybe the third case of TSDoc not aligning with the actual type signature.

While I do think using the TSDoc comment format to annotate functions and parameters makes a lot of sense (and displays really nicely in VSCode), I'm wondering if we need to use @param notation at all.

In particular, if I had placed this multi-line TSDoc comment above the type parameter in the actual function signature — and omitted the symbol name, it still seems to show up real nice in VSCode. e.g.:

Rather than:

/** * Returns the average of two numbers. * * @param x - The first input number * @param y - The second input number * @returns The arithmetic mean of `x` and `y` * */ function getAverage(x, y) { return (x + y) / 2.0; }

We might/could use:

/** * Returns the average of two numbers. * * @returns The arithmetic mean of `x` and `y` * */ function getAverage( /** * The first input number */ x: number, /** * The second input number */ y: number, ) { return (x + y) / 2.0; }

Again, this seems to look nice in VSCode and shows as a nice-hover over on the parameter usages and Intelli-spidey-senses, however, TSDoc itself, seems to not compute it on its own. That would be a sad if we ever generated documentation using that, but perhaps that's a feature that's bound to come to TSDoc eventually (or already has in some other form, but didn't seem to work on their playground.

Anyhow, just food for thought! (On which I'm fishing for thoughts)

I don't feel too strongly about this, though I agree in your second example that it certainly maintains better! I'm personally more interested in the generated documentation use case for this - maybe we can open an issue against TSDoc to support this?

Interesting that VSCode already does, I wonder if they're using TSDoc at all or possibly an extension of it? Could be worth investigating if we want to pursue!

Hmm… the second one maintains better, I agree, but I find the code harder to read. Between that, TSDoc understanding @param out of the box, and VSCode adding @params automatically, I think I prefer the @parametric style. I don't feel dogmatically about it at all. I also think it's something we can decide on a case-by-case basis (especially if we find a flag or something we can flip in TSDoc to have it pick up VSCode style comments).

In this case, we could have avoided the issue by not having a @param at all, right? The description "an instance of ApolloServer" tells you nothing that the types don't already.

v3/src/transports/http/handler.ts

v3/src/transports/http/transport.ts

queerviolet · 2019-12-03T12:49:00Z

v3/src/transports/http/transport.ts

+    error.locations.length
+  ) {
+    if (Array.isArray(error.nodes) && error.nodes.length) {
+      return 400; // Validation error


Do they have error codes we could use?

v3/src/transports/http/transport.ts

v3/src/transports/http/handler.ts

abernix

Really appreciate the feedback! I've responded within and I'll follow-up with some adjustments later.

v3/src/execution/index.ts

abernix · 2019-12-03T14:36:38Z

v3/src/transports/http/handler.ts

+ * a `RequestHandler` that can be used with Node.js' `http.createServer`, or
+ * Express' `app.use`.
+ *
+ * @param apollo An instance of `ApolloServer`


Good catch, it is! Though the previous implementation I had did leverage an instance of ApolloServer, I changed that to align with the expectations of the new processGraphqlRequest from #3428

I'll adjust this, but speaking to the maintainability of these TSDoc notations though: This mismatch marks, in my experience on this new code-base, maybe the third case of TSDoc not aligning with the actual type signature.

While I do think using the TSDoc comment format to annotate functions and parameters makes a lot of sense (and displays really nicely in VSCode), I'm wondering if we need to use @param notation at all.

In particular, if I had placed this multi-line TSDoc comment above the type parameter in the actual function signature — and omitted the symbol name, it still seems to show up real nice in VSCode. e.g.:

Rather than:

/** * Returns the average of two numbers. * * @param x - The first input number * @param y - The second input number * @returns The arithmetic mean of `x` and `y` * */ function getAverage(x, y) { return (x + y) / 2.0; }

We might/could use:

/** * Returns the average of two numbers. * * @returns The arithmetic mean of `x` and `y` * */ function getAverage( /** * The first input number */ x: number, /** * The second input number */ y: number, ) { return (x + y) / 2.0; }

Again, this seems to look nice in VSCode and shows as a nice-hover over on the parameter usages and Intelli-spidey-senses, however, TSDoc itself, seems to not compute it on its own. That would be a sad if we ever generated documentation using that, but perhaps that's a feature that's bound to come to TSDoc eventually (or already has in some other form, but didn't seem to work on their playground.

Anyhow, just food for thought! (On which I'm fishing for thoughts)

v3/src/transports/http/handler.ts

v3/src/transports/http/transport.ts

abernix · 2019-12-03T14:54:34Z

v3/src/transports/http/transport.ts

+    error.locations.length
+  ) {
+    if (Array.isArray(error.nodes) && error.nodes.length) {
+      return 400; // Validation error


Not yet. We previously had ValidationError and ParseError, but those have not re-surfaced in the new machinery yet. cc @trevor-scheer something we'll need.

Co-Authored-By: Ashi Krishnan <ashi@queerlyamorous.org>

trevor-scheer

Leaving a partial review since I'm taking off for the evening and currently tied up with a gateway bug that may occupy my time tomorrow as well. I'll revisit and try to pick up where I left off.

trevor-scheer · 2019-12-03T19:35:02Z

v3/src/transports/http/handler.ts

+ * a `RequestHandler` that can be used with Node.js' `http.createServer`, or
+ * Express' `app.use`.
+ *
+ * @param apollo An instance of `ApolloServer`


I don't feel too strongly about this, though I agree in your second example that it certainly maintains better! I'm personally more interested in the generated documentation use case for this - maybe we can open an issue against TSDoc to support this?

Interesting that VSCode already does, I wonder if they're using TSDoc at all or possibly an extension of it? Could be worth investigating if we want to pursue!

v3/src/transports/http/handler.ts

trevor-scheer

Really love this, @abernix. I only had some minor comments to add. The extensive documentation is much appreciated!

Look forward to seeing the tests you're interested in adding as well.

I think there are a few places where the new execution work could improve the outcome here. Should we target those changes sooner rather than later? Namely, XyzError types and returning AsyncIterable from processGraphqlRequest - but maybe there are some other improvements that aren't crossing my mind atm as well.

v3/src/transports/http/handler.ts

v3/src/transports/http/transport.ts

trevor-scheer · 2019-12-06T19:51:41Z

v3/src/transports/http/transport.ts

+    error.locations.length
+  ) {
+    if (Array.isArray(error.nodes) && error.nodes.length) {
+      return 400; // Validation error


Definitely! Any reason in particular for the hesitations on using apollo-server-errors? Are we interested in having those live within the v3 directory / @apollo/server package?

Worth noting, we do have a breaking change to make on the ApolloError type, so leaving it all behind is really not a terrible option.

v3/src/types/index.ts

Note that this was only a lie because the implementation has been temporarily swapped out. The correct solution, as I've noted in this code comment, should absolutely not be requiring a `schema` be passed in, but rather a reference to a function that can be used to actually execute the operation and receive back a `GraphQLResponse`. Ref: #3576 (comment)

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r353153479

This won't remain, but this changes to address the feedback. Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r353425116

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r354963931

A very valid use of types in TSDoc, since TypeScript doesn't support this annotation more explicitly right now!

@trevor-scheer

This doesn't seem to provide as clear auto-complete when actually displaying the auto-complete/intellisense details when utilizing the function. In other words, when typing `generatedResponse({ response: ` in VS Code, this doesn't tell me the details I'd hope for. cc @trevor-scheer, help? I thought we'd considered this approach and found it to be better but maybe I'm doing something wrong now.

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r355008437

If either of these extra defensive conditionals are met, there is something very much mis-wired in the end-user's usage of this handler. Such a mis-configuration can only be raised through native error throwing. Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r354988001

This partially reverts commit d90f2ec, which had removed what is likely not the correct class-based approach, but somewhat aligns with the new lift-off and new execution approaches. Call it a hybrid.

These were used by a WIP `userContext` property on the `ApolloServer` class, but that class is still very much a work in progress.

This also changes it to be the default export from `transports/http/handler`.

abernix

@trevor-scheer @queerviolet I think this is ready for another round of feedback! It should be in a much more stable state and now has a fair number of tests to back that claim up with. Further, it now utilizes some of the other components that have already landed, like the inlined execution from #3656.

I've also updated the original PR description and left a number of comments throughout. The PR body changes include the addition of several TODOs worth consideration, and those more accurately reflect the next steps to be addressed — though I think they'll all be follow-up PR concerns.

There are also some less critical TODOs sprinkled around the code this PR adds, but I don't think any of those are necessarily blocking us from shipping this to the (non-released, non-stable) release-3.x branch and iterating on it further.

Feedback very much appreciated!

v3/src/execution/index.ts

abernix · 2020-01-23T22:39:42Z

v3/src/execution/index.ts


 // TODO(AS3) I'm not sure if this is execution.  Perhaps, a top-level export.
 export { GraphQLSchemaModule } from 'apollo-graphql';

 export { Context, ContextFunction } from 'apollo-server-core';

-/** Options for {@link processGraphQLRequest} */
-interface ProcessRequestInput<TContext extends Record<string, any>> {
+export type UserContext = Record<string, any>;


Whether or not this remains the precise typing, or whether or not this file is the place it lives forever, I hope this will eventually become the only declaration of "the type which indicates the user's context", rather than re-declaring Record<string, any> in literally dozens of places even before the new execution work from #3423 (which this is now based on) landed.

v3/src/execution/index.ts

abernix · 2020-01-23T22:46:52Z

v3/src/execution/index.ts

-interface ExecutionInput<
-  TContext extends Record<string, any> = Record<string, any>
-> {
+interface ExecutionInput<TContext extends UserContext = UserContext> {


ExecutionInput does accept a type parameter of the UserContext, but it's been removed in many places here as it was being under-utilized and is in need of additional attention to consider its role in the scheme of things and how we want to pass it around. Therefore, we'll default to UserContext in many places for now. Thoughts appreciated!

abernix · 2020-01-23T22:50:58Z

v3/src/transports/http/__tests__/transport.test.ts

+      ],
+      throwForbiddenError() {
+        // TODO(AS3) What should we do with these?
+        throw new ForbiddenError("Not allowed!");


This isn't resolved, but let's nail down the role of #3645 here and consider how we expect like this within resolvers to affect the status code. Considering a @defer'd resolver may not resolve until long after the headers are sent, I don't think it's necessarily practical to change the status code based these existing apollo-server-errors classes we've previously provided.

v3/src/transports/http/transport.ts

abernix · 2020-01-23T23:40:23Z

v3/src/transports/http/transport.ts

+   * In the future, GraphQL execution should return an `AsyncIterable`. However,
+   * today it returns a `Promise`, so we'll coerce it into an `AsyncIterable`
+   * with a generator function implemented on a `Symbol.asyncIterator` property.
+   */


With the inlining of execution in #3423, can we actually change it to be an AsyncIterable now?

I'd love to pair on that with you and learn about AsyncIterables in the process. For now I can't answer the question but it seems reasonable to me!

abernix · 2020-01-23T23:42:56Z

v3/src/transports/http/transport.ts

+    error.locations.length
+  ) {
+    if (Array.isArray(error.nodes) && error.nodes.length) {
+      return 400; // Validation error


Should we use #3645 for these errors? Can we make ValidationError implement the new errors in a way where they remain compatible with existing users, e.g., who might be doing instanceof checks against ValidationError and ParsingErrors in their plugin hooks?

abernix · 2020-01-23T23:44:17Z

v3/src/types/index.ts

+  & ExecutionResult<ExecutionResultDataDefault>
+  & ResponseExtensions;
+
+export type GraphQLResponseWithPreExecutionErrors =


This type is not currently used, but is included for completeness.

v3/src/execution/index.ts

v3/src/transports/http/__tests__/handler.test.ts

trevor-scheer · 2020-01-27T18:12:53Z

v3/src/transports/http/__tests__/handler.test.ts

+    describe.each<RequestOptions["method"]>([
+      "GET",
+      "POST",
+    ])(


v3/src/transports/http/__tests__/handler.test.ts

trevor-scheer · 2020-01-27T18:20:25Z

v3/src/transports/http/__tests__/handler.test.ts

+        // This also tests the behavior of the `initHandlerWithParams` helper.
+        it("reads streams of the request body appropriately", async () => {
+          // Only one of the two scenarios should be triggered.
+          expect.assertions(1);


This could be removed, unless you feel like it's adding some clarity to the test case, which I could understand. Effectively, it's validating that the if / else if construct works as expected.

It's true that it's not necessary in this scenario, however, more so than added clarity, I believe it's still defensive since it would force a developer who adds a new method (e.g. DELETE) to the describe.each list above to add an appropriate expectation to this block.

Not adding that line would cause this test to pass as neither of its conditional states would be met. Do you agree?

Ah, that makes sense. This tidbit would be a great addition to the comment above so I understand that zero assertions could occur in the case that a new method were added, and we'd want to guard against that.

trevor-scheer · 2020-01-27T19:23:18Z

v3/src/transports/http/__tests__/utils.test.ts

+    })
+    // We'll actually return the `Promise` of the handler itself here to
+    // un-wrap what would otherwise be a `Promise` of a `Promise`.
+    .then((resolved) => resolved);


This throws me for a bit of a loop, but I can't work around it. I'd love to understand why this is necessary, but more importantly, why the following doesn't yield the same result (my understanding is off somewhere).

const handlerPromise = Promise.resolve( new Promise<ReturnType<AsyncRequestListener>>((resolve) => { handlerPromiseResolve = resolve; }) );

v3/src/execution/index.ts

trevor-scheer · 2020-01-27T19:32:26Z

v3/src/transports/http/handler.ts

+  return parsedBody;
+}
+
+export const __testing__ = {


I really like this style for minimizing the API exposure for testing purposes.

Me too! One comment on it: Where I've done it, I find it a bit easier to include the module name in these. So it's like export const __handler_testing__, so that if a test imports multiple testpoints, they're easily distinguished.

Full credit for the pattern goes to @queerviolet!

I thought about adopting the pattern which included the module name, but I thought it would probably atrophy in the case that modules were ever moved, and also thought it might be an inappropriate mixing of concerns to import multiple multiple modules into a single test file (which are often 1:1, e.g. util.test.ts to util.ts).

In the end, I didn't find the ergonomic of using an aliased import in the case that it was necessary to import from multiple modules problematic, and a healthy reminder of what boundary was being crossed.

Still happy to change it, though. 😉

trevor-scheer · 2020-01-27T19:35:04Z

v3/src/transports/http/transport.ts

+   * In the future, GraphQL execution should return an `AsyncIterable`. However,
+   * today it returns a `Promise`, so we'll coerce it into an `AsyncIterable`
+   * with a generator function implemented on a `Symbol.asyncIterator` property.
+   */


I'd love to pair on that with you and learn about AsyncIterables in the process. For now I can't answer the question but it seems reasonable to me!

trevor-scheer · 2020-01-27T19:38:22Z

v3/src/transports/http/utils.ts

+  try {
+    const parsed = JSON.parse(input);
+    if (typeof parsed !== "object") {
+      throw new Error();


Should this have a TODO? Mostly curious if we want to be more specific than throwing a generic Error with no message.

The intent here to leverage the logic in the catch block, which doesn't use any more specific message, but rather the errMsg which the parseJsonInputAsObject was passed during invocation.

Does that seem reasonable since the entire intent of the function is to make such an assertion and raise the specified message?

Alternatively, I could throw null or throw undefined, or I could just change parsed to a let (declared above the try) and assert that typeof parsed !== "object" after the catch (with another throw new SyntaxError(errMsg)).

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r371376751

@trevor-scheer

The pattern here made sense in a previous incarnation of the way this test was employed, but that's no longer the case. Without this critical fix, as noted by @trevor-scheer[1], a resolution (as opposed to a rejection) in the `httpHandler` would have not triggered the `catch` and its associated `expect`-tation without an `expect.assertions`, which is unnecessary using `.rejects.toThrow` as we do in other places in this new code. [1]: https://github.com/apollographql/apollo-server/pull/3576/files#r371399440

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r374799988

trevor-scheer · 2020-02-20T01:16:34Z

v3/src/transports/http/__tests__/handler.test.ts

+        // This also tests the behavior of the `initHandlerWithParams` helper.
+        it("reads streams of the request body appropriately", async () => {
+          // Only one of the two scenarios should be triggered.
+          expect.assertions(1);


Ah, that makes sense. This tidbit would be a great addition to the comment above so I understand that zero assertions could occur in the case that a new method were added, and we'd want to guard against that.

alfaproject · 2020-06-15T20:42:59Z

Was so looking forward to this but it looks abandoned? ):

glasser · 2022-10-07T22:55:35Z

We didn't end up using this PR directly to achieve its goals, but have done so with a different implementation in AS4 (currently a Release Candidate).

abernix added 7 commits December 2, 2019 22:42

v3: Begin to introduce new transport.

40bc81f

Remove some original implementation details which are no longer relev…

d90f2ec

…ant. We started off building off some of the Apollo Server 2.x base, and that is no longer as feasible with our new directions!

Rename tsdoc which was for a different (imported) symbol, rather than…

2f5e7ab

… local.

Remove TData until I understand it better!

c9abb68

Apply guard on data to tests which seems to make sense.

47f52a3

TypeScript was suggesting this, and I believe that it makes sense!

Rename badly named test file.

8f4e486

My fault! Not only should this match the file that it tests (transport.ts), but it should also have `.test.ts` in the name for consistency with other test files, but also for a future where we quash the `__test__` directory entirely.

abernix requested review from queerviolet and trevor-scheer December 2, 2019 21:08

abernix commented Dec 2, 2019

View reviewed changes

queerviolet reviewed Dec 3, 2019

View reviewed changes

abernix commented Dec 3, 2019

View reviewed changes

Apply suggestions from code review

3cce418

Co-Authored-By: Ashi Krishnan <ashi@queerlyamorous.org>

trevor-scheer reviewed Dec 4, 2019

View reviewed changes

jbaxleyiii self-assigned this Dec 4, 2019

trevor-scheer reviewed Dec 6, 2019

View reviewed changes

v3/src/transports/http/handler.ts Outdated Show resolved Hide resolved

trevor-scheer reviewed Dec 6, 2019

View reviewed changes

abernix added 13 commits December 16, 2019 20:38

Remove trailing space.

9df5a54

Switch httpHandler to use easier to read function call.

e203baa

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r353153479

Use isSchema rather than instanceof check.

04f7745

This won't remain, but this changes to address the feedback. Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r353425116

Address clarity-of-code comment w/r/t unclear return practice.

2862d98

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r354963931

Add a @throws TSDoc annotation for possible result of jsonBodyParse.

c989b92

A very valid use of types in TSDoc, since TypeScript doesn't support this annotation more explicitly right now!

Remove unnecessary ternary operators.

353f1f1

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r355008437

Remove unused experimental_ interfaces.

627d8a5

Re-introduce ApolloServer.prototype.executeOperation, for now.

eae9bb0

This partially reverts commit d90f2ec, which had removed what is likely not the correct class-based approach, but somewhat aligns with the new lift-off and new execution approaches. Call it a hybrid.

Switch to v3 type for VariableValues.

a1be448

Remove unused (commented out) type definitions.

5e572d4

abernix added 2 commits January 24, 2020 01:57

Remove unused Context and ContextFunction types.

b031d6c

These were used by a WIP `userContext` property on the `ApolloServer` class, but that class is still very much a work in progress.

Export the httpHandler as a top-level export.

185965c

This also changes it to be the default export from `transports/http/handler`.

abernix commented Jan 24, 2020

View reviewed changes

abernix requested review from trevor-scheer and queerviolet January 24, 2020 00:32

Typo-corrections from my own code review.

a00ec04

trevor-scheer reviewed Jan 27, 2020

View reviewed changes

abernix added 2 commits February 4, 2020 17:32

Remove unused export of GraphQLRequest and GraphQLResponse.

7bd1949

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r371376751

abernix force-pushed the abernix/release-3.x-transport branch from 5abc7c4 to 01be050 Compare February 4, 2020 16:52

tests: Feedback: Adjust types to properly indicate Promise of nothing.

b2ec085

Ref: https://github.com/apollographql/apollo-server/pull/3576/files#r374799988

queerviolet approved these changes Feb 10, 2020

View reviewed changes

trevor-scheer approved these changes Feb 20, 2020

View reviewed changes

This was referenced Feb 24, 2020

Consider providing an option to disable the "x-powered-by" header #3812

Closed

apollo-server: Disable "x-powered-by" header #3821

Merged

This was referenced Apr 22, 2020

[FIX] Fix fastify startup deprecation warning #3561

Closed

[FIX] - Fastify Deprecated Warning #3527

Closed

beforeHandler to preHandler #3674

Closed

abernix added this to the Release 3.x milestone Dec 7, 2020

abernix changed the base branch from release-3.x to release-3.0 February 5, 2021 06:50

abernix assigned abernix and unassigned jbaxleyiii Feb 5, 2021

glasser removed this from the Release 3.x milestone May 2, 2021

abernix marked this pull request as draft May 7, 2021 10:25

glasser closed this Oct 7, 2022

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Scaffold out new transport mechanisms #3576

Scaffold out new transport mechanisms #3576

Conversation

abernix commented Dec 2, 2019 • edited Loading

TODO (in follow-up PRs)

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

queerviolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

queerviolet Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfaproject commented Jun 15, 2020

glasser commented Oct 7, 2022

abernix commented Dec 2, 2019 •

edited

Loading

queerviolet Dec 4, 2019 •

edited

Loading