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

Enable chaining for middlewares and afterwares in the network interface - Issue 564 #860

Merged
merged 4 commits into from Nov 2, 2016

Conversation

ghost
Copy link

@ghost ghost commented Oct 31, 2016

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • 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
  • Update CHANGELOG.md with your change
  • Add your name and email to the AUTHORS file (optional)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@ghost ghost changed the title Enable chaining for middlewares and afterwares in the network interface - Issue 564 WIP - Enable chaining for middlewares and afterwares in the network interface - Issue 564 Oct 31, 2016
@ghost
Copy link
Author

ghost commented Oct 31, 2016

Hi,
I'm trying to solve #564. Have a look at this PR and tell me what you think.
Remarks :

  1. If I'm not mistaking, it should also possible to chain use() and useAfter() in the same call. I might need to also add a test for that. Right now, my patch only tests use(...).use(...) and useAfter(...).useAfter(...). Please tell me if such test is relevant.
  2. As this PR adds a new feature, I need to update the docs. I will, but first I'd like to know if the PR looks good.

Regards

Copy link
Contributor

@helfer helfer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot! Just one tiny change requested.

@@ -3,6 +3,7 @@
Expect active development and potentially significant breaking changes in the `0.x` track. We'll try to be diligent about releasing a `1.0` version in a timely fashion (ideally within 3 to 6 months), to signal the start of a more stable API.

### vNext
- **new Feature**: In network interface, enable chaining the `use` and `useAfter` functions, respectively. [PR #860](https://github.com/apollostack/apollo-client/pull/860) [Issue #564](https://github.com/apollostack/apollo-client/issues/564)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify this to "Enable chaining of use and useAfter function calls in network interface"?

@ghost
Copy link
Author

ghost commented Nov 1, 2016

Just added a new commit that includes the required change + two new tests for testing use(...).useAfter(...) and useAfter(...).use(...) chaining.
I will now work on a PR on the docs. Stay tuned ...

@ghost
Copy link
Author

ghost commented Nov 1, 2016

Here is a link to a PR on apollostack/core-docs which documents this feature :
https://github.com/apollostack/core-docs/pull/218

@ghost ghost changed the title WIP - Enable chaining for middlewares and afterwares in the network interface - Issue 564 Enable chaining for middlewares and afterwares in the network interface - Issue 564 Nov 1, 2016
@ghost
Copy link
Author

ghost commented Nov 1, 2016

For me, this PR is ready to be merged but tell me if some changes need to be done.

@helfer
Copy link
Contributor

helfer commented Nov 2, 2016

Looks good to me. Thanks a lot @oricordeau !

@helfer helfer merged commit 0290b7d into apollographql:master Nov 2, 2016
@ghost ghost deleted the issue-564 branch November 2, 2016 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants