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

cleanup optimistic updates tests #3713

Merged
merged 14 commits into from
Aug 17, 2018

Conversation

joshribakoff
Copy link
Contributor

please reference an issue where a consensus about the design was reached (not necessary for small changes)

#3712

@apollo-cla
Copy link

@joshribakoff: 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/

@apollo-cla
Copy link

apollo-cla commented Jul 20, 2018

Fails
🚫

No CHANGELOG added.

Warnings
⚠️

❗ Big PR

Generated by 🚫 dangerJS

@joshribakoff
Copy link
Contributor Author

@hwillson

I was able to flatten the complex test case! I find this much easier to understand. Please see Before and After.

I first tried to use zen-observable.reduce for this, but it did not work [because it messed with the internal state of QueryObservable]. I'll only use rxjs for the tests, not in any production code for the scope of this PR...

Please let me know if this looks good I'll finish up the rest of the optimist tests!

@joshribakoff joshribakoff changed the title [WIP] cleanup optimistic updates tests cleanup optimistic updates tests Jul 23, 2018
@joshribakoff
Copy link
Contributor Author

joshribakoff commented Jul 23, 2018

I finished the will handle dependent updates refactoring. All tests are passing. Circle CI simply warns there is no changelog.

While working on these tests, I ran into this odd issue I wrote up here.

#3723

This PR could now be merged and I can do another PR later on to flatten the rest of the tests.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @joshribakoff, and sorry for the delay getting this merged! Everything looks👌, so we'll have this merged shortly. Thanks again!

@hwillson hwillson merged commit 6f579ea into apollographql:master Aug 17, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 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.

None yet

3 participants