Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apollo-server-express: don't promise to support Connect #5173

Merged
merged 1 commit into from
May 6, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented May 4, 2021

Connect is a pretty old JS web framework; most users (other than Meteor) moved
to Express many years ago.

This PR stops advertising apollo-server-express supports Connect. It doesn't
actively remove the lines in apollo-server-express which exist for Connect
compatibility and it does continue to test against Connect. However, it does
state that we may choose to remove that compatibility within Apollo Server 3.

Fixes #5172.

@glasser glasser requested a review from abernix May 4, 2021 23:03
@glasser
Copy link
Member Author

glasser commented May 4, 2021

@abernix Curious about your opinion here. I feel like it is about time that we should feel comfortable actually using the Express API when it helps us, and that if people (eg Meteor users) really need connect support then they can write a little apollo-server-connect.

Base automatically changed from glasser/upgrade-ts-typecheck-tests to release-3.0 May 5, 2021 17:01
@glasser
Copy link
Member Author

glasser commented May 5, 2021

Another middle ground would be to leave the tests in place but still remove the references to it in docs, and only remove the tests once we are motivated to make a change that would break them by requiring Express-specific features. ie, shifting the policy from "this package definitely supports connect" to "this package is known to support connect, but we may explicitly choose to remove that support during AS3".

@glasser
Copy link
Member Author

glasser commented May 5, 2021

I split this into two commits: the first is primarily a doc change, and the second actually stops running tests. My inclination right now is that I'd like to merge only the first commit; curious your thoughts.

@glasser
Copy link
Member Author

glasser commented May 5, 2021

(On the other hand, maybe maintaining Connect support is very little burden, and we're more likely to execute the full #3184 sooner than we would use fancy Express-specific features in ASE.)

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to stop advertising Connect support and to stop testing it.

I suspect it will "just" keep working indefinitely just the same; I think it's pretty unlikely that Express and Connect would become incompatible and I thought they were largely wholly compatible with each other anyhow (with maybe the exception of their res.end handler or something at one point?).

You're not wrong that the burden of maintaining seems like it's just a matter of keeping the package around for testing (since they're otherwise the same handler today, right?), but I think it's less likely that we would fix any incompatibility that does arise if those tests started failing. Hopefully if that does happen, #3184 will have already happened.

@glasser
Copy link
Member Author

glasser commented May 6, 2021

I think I have enough of a soft spot for Meteor that I'd like to not unknowingly break using AS with it. My understanding is the major difference between Express and Connect is that Connect does not have its own request and response types and just uses Node's? I do think that using Express request/response features is something I can see myself (as somebody with perhaps a less deep intuitive understanding of the APIs involved) doing by accident.

Connect is a pretty old JS web framework; most users (other than Meteor) moved
to Express many years ago.

This PR stops advertising apollo-server-express supports Connect.  It doesn't
actively remove the lines in apollo-server-express which exist for Connect
compatibility and it does continue to test against Connect. However, it does
state that we may choose to remove that compatibility within Apollo Server 3.

Fixes #5172.
@glasser glasser changed the title apollo-server-express: don't explicitly support Connect apollo-server-express: don't promise to support Connect May 6, 2021
@glasser glasser enabled auto-merge (squash) May 6, 2021 16:22
@glasser glasser merged commit a29db0e into release-3.0 May 6, 2021
@glasser glasser deleted the glasser/no-connect branch May 6, 2021 16:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 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.

2 participants