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

Pass ctx instead of ctx.request to koa options function #154

Merged
merged 3 commits into from
Oct 1, 2016

Conversation

HriBB
Copy link
Contributor

@HriBB HriBB commented Sep 21, 2016

This is a breaking change. We need to mention this in the changelog.

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

@helfer
Copy link
Contributor

helfer commented Sep 21, 2016

@HriBB can you add that to the changelog? I think it's a good change, but it's a pretty major breaking change for the Koa integration.

@HriBB
Copy link
Contributor Author

HriBB commented Sep 21, 2016

Done!

@mikeifomin
Copy link

mikeifomin commented Sep 22, 2016

@HriBB
I think you forgot to fix an interface for KoaApolloOptionsFunction

@HriBB
Copy link
Contributor Author

HriBB commented Sep 22, 2016

@mikeifomin you're right, thanks! @helfer should I add a test for that?

@helfer
Copy link
Contributor

helfer commented Sep 22, 2016

@HriBB I don't even know how we could test for missing (type) exports. If you figure out a way to do that, I definitely want to know!

@HriBB
Copy link
Contributor Author

HriBB commented Sep 22, 2016

Should be fixed now, @mikeifomin can you check? @helfer yeah I'm not very experienced with tests. I proposed that because I ran tests after fixing the ctx and all tests passed, so I assumed my change was OK.

@helfer
Copy link
Contributor

helfer commented Sep 30, 2016

@HriBB @mikeifomin is this ready to merge?

@HriBB
Copy link
Contributor Author

HriBB commented Sep 30, 2016

@helfer yes!

@helfer helfer merged commit 47fd535 into apollographql:master Oct 1, 2016
@stubailo
Copy link
Contributor

So how do we release this? Bump to v0.4?

@helfer
Copy link
Contributor

helfer commented Oct 13, 2016

@stubailo yeah, we could release this before the monorepo, which would then be version 0.5

@stubailo
Copy link
Contributor

OK, will do.

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