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

fix: separate type parameters for input and output in ContextFunction #2346

Conversation

disintegrator
Copy link

@disintegrator disintegrator commented Feb 21, 2019

The type of context when using a ContextFunction is incorrect as of [email protected]. Both the input context AND output context had to be ExpressContext. This PR redefines ContextFunction to have two type parameters: an input context type (in this case it's ExpressContext) and output context type to be supplied by consumer.

Fixes #1593

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@apollo-cla
Copy link

@disintegrator: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@cheapsteak
Copy link
Member

Thanks so much for the PR @disintegrator !
I've pulled it down and tested that this is in fact a more correct typing of that function

@MichalLytek
Copy link

Yeah, I need that fix too!
I've tried to fix the TS errors but the req and res requirement is just a bug.

@disintegrator
Copy link
Author

Hey thanks y’all. I’m just waiting to hear if there’ll be any issues signing this CLA as I did the work during work hours - I’m not clear on how CLAs work. I’ll sort out first thing tomorrow (London time)

@cheapsteak
Copy link
Member

Hi folks!

We've just released 2.4.4 that includes the changes from #2350 and should address the issue that this PR also addresses
The main difference is that, at least for now, we won't be opening up the value of the produced context variable open to be passed in. Adding type arguments to the main ApolloServer constructor would be a fairly significant API decision, it may be something we'd need to do eventually but would require a bit more thought and discussion (e.g. should the type of Context be the first type argument?)

Please let us know if there was anything else that's covered by this PR that we missed in the 2.4.4 release, in the mean time I'll close this issue as part of cleanup

Thanks again to @disintegrator for your time in putting this PR together and especially the description, it's tremendously appreciated 🙏

@cheapsteak cheapsteak closed this Feb 21, 2019
@disintegrator disintegrator deleted the fix-ContextFunction-type branch February 21, 2019 18:41
@disintegrator
Copy link
Author

@cheapsteak Ye I figured it’d be a big change when I created the PR. I’m glad to see you arrived at a less invasive solution. Will give it a go soon.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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.

4 participants