Re-land "Expose composed middleware via getMiddleware()" #3047
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aside from avoiding unnecessary dependencies, by using this Router, we avoid the `apollo-server` base package from behaving different than it has in the past. Specifically, `apollo-server` uses `/` as the default path, but it does so in a wild-card way which serves GraphQL requests on _any_ path. That means that `/graphqlllll` and `/graphql` both also served the GraphQL Playground interface, and also responded to GraphQL execution requests. Since some may be leveraging that behavior, we should preserve it, if we can.
abernix
force-pushed
the
abernix/reland-2435
branch
from
c72277b
to
16a8462
Compare
|
Released in Apollo Server 2.9.0 (and associated integrations!) Finally! |
This was referenced Aug 23, 2019
Closed
abernix
added a commit
that referenced
this pull request
Aug 31, 2019
The [literal `import`-ing of `express` in `ApolloServer.ts`][1] isn't new but, prior to #3047, it had only been a typing dependency — not an actual runtime dependency — since the TypeScript compiler doesn't emit `require`s when only types are utilized. To see this first hand, see [the emitted `dist/ApolloServer.js` in `[email protected]`][2] compared to [the same file in `[email protected]`][3]. (Hard to decipher, but check around like 15 and search for `"express"` in the second link.) Now that we actually utilize `express.Router` to build the composition of middleware in #3047 , it's true that `express` is no longer just a type dependency, but does warrant being a regular dependency as well! Since this has never been the case before during the entirety of the v2 line, I'm a bit concerned about introducing it now, but it seems other integrations like `apollo-server-koa` already have similar requirements going back to its introduction in [#1282][4] https://github.com/apollographql/apollo-server/blob/92ea402a90bf9817c9b887707abbd77dcf5edcb4/packages/apollo-server-koa/package.json#L41 Furthermore, we already have similar direct dependencies on other packages which we use directly, like [`cors`][5] and [`body-parser`][6]: https://github.com/apollographql/apollo-server/blob/ff3af66a0f3c63bfb056feca82fc7e7b7592b4a5/packages/apollo-server-express/package.json If this turns out to be problematic, we could consider declaring it only in `peerDependencies`, but those come with their own [share of confusion][7]. [1]: https://github.com/apollographql/apollo-server/blob/6d9c3b8c97/packages/apollo-server-express/src/ApolloServer.ts#L1 [2]: https://unpkg.com/[email protected]/dist/ApolloServer.js [3]: https://unpkg.com/[email protected]/dist/ApolloServer.js [4]: #1282: [5]: https://npm.im/cors [6]: https://npm.im/body-parser [7]: npm/rfcs#43 Fixes: #3238
abernix
added a commit
that referenced
this pull request
Aug 31, 2019
The [literal `import`-ing of `express` in `ApolloServer.ts`][1] isn't new but, prior to #3047, it had only been a typing dependency — not an actual runtime dependency — since the TypeScript compiler doesn't emit `require`s when only types are utilized. To see this first hand, see [the emitted `dist/ApolloServer.js` in `[email protected]`][2] compared to [the same file in `[email protected]`][3]. (Hard to decipher, but check around like 15 and search for `"express"` in the second link.) Now that we actually utilize `express.Router` to build the composition of middleware in #3047 , it's true that `express` is no longer just a type dependency, but does warrant being a regular dependency as well! Since this has never been the case before during the entirety of the v2 line, I'm a bit concerned about introducing it now, but it seems other integrations like `apollo-server-koa` already have similar requirements going back to its introduction in [#1282][4] https://github.com/apollographql/apollo-server/blob/92ea402a90bf9817c9b887707abbd77dcf5edcb4/packages/apollo-server-koa/package.json#L41 Furthermore, we already have similar direct dependencies on other packages which we use directly, like [`cors`][5] and [`body-parser`][6]: https://github.com/apollographql/apollo-server/blob/ff3af66a0f3c63bfb056feca82fc7e7b7592b4a5/packages/apollo-server-express/package.json If this turns out to be problematic, we could consider declaring it only in `peerDependencies`, but those come with their own [share of confusion][7]. [1]: https://github.com/apollographql/apollo-server/blob/6d9c3b8c97/packages/apollo-server-express/src/ApolloServer.ts#L1 [2]: https://unpkg.com/[email protected]/dist/ApolloServer.js [3]: https://unpkg.com/[email protected]/dist/ApolloServer.js [4]: #1282: [5]: https://npm.im/cors [6]: https://npm.im/body-parser [7]: npm/rfcs#43 Fixes: #3238
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit d05aceb which was originally #2435, but was reverted in 52ab22e (#3046) as a precaution, prior to releasing 2.7.0. This attempts to reland it.