Skip to content

apollo-server-express: Add direct dependency on express.#3239

Merged
abernix merged 3 commits into
masterfrom
abernix/add-express-dependency-apollo-server-express
Aug 31, 2019
Merged

apollo-server-express: Add direct dependency on express.#3239
abernix merged 3 commits into
masterfrom
abernix/add-express-dependency-apollo-server-express

Conversation

@abernix

@abernix abernix commented Aug 31, 2019

Copy link
Copy Markdown
Member

The literal import-ing of express in ApolloServer.ts 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 requires when only types are utilized.

To see this first hand, see the emitted dist/ApolloServer.js in apollo-server-express@2.8.2 compared to the same file in apollo-server-express@2.9.0. (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

Furthermore, we already have similar direct dependencies on other packages which we use directly, like cors and body-parser:

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.

Fixes: #3238

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
`apollo-server-express@2.8.2`][2] compared to [the same file in
`apollo-server-express@2.9.0`][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/apollo-server-express@2.8.2/dist/ApolloServer.js
[3]: https://unpkg.com/apollo-server-express@2.9.0/dist/ApolloServer.js
[4]: #1282:
[5]: https://npm.im/cors
[6]: https://npm.im/body-parser
[7]: npm/rfcs#43
Fixes: #3238
@abernix abernix merged commit 1084d17 into master Aug 31, 2019
@abernix abernix deleted the abernix/add-express-dependency-apollo-server-express branch August 31, 2019 21:26
@abernix abernix requested a review from trevor-scheer August 31, 2019 21:27
abernix added a commit that referenced this pull request Sep 1, 2019
@abernix abernix added this to the Release 2.9.3 milestone Sep 1, 2019
@trevor-scheer

Copy link
Copy Markdown
Contributor

Just making note that our users are now limited to express>=4.17.1 && express<5 (still alpha). I don't think this is a cause for concern, though.

@abernix

abernix commented Sep 4, 2019

Copy link
Copy Markdown
Member Author

@trevor-scheer They're not necessarily limited to that range, though an incompatible version would result in a duplicated express dependency if the package manager wasn't able to de-dupe based on compatible versions. That said, the version of express that they use would need to have a compatible Router. I suspect that all versions within v4 should meet that requirement.

The express has had v5 in prerelease (e.g. alpha) for a while now, and I'm hoping that Apollo Server 3.x's transports (which will obsolete this getMiddleware code entirely; see #3184) will be the ultimate solution for compatibility with express >=5.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 22, 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.

[apollo-express-server] Missing dependency on express

2 participants