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

New skip method #253

Merged
merged 3 commits into from Oct 7, 2016

Conversation

Projects
None yet
4 participants
@jbaxleyiii
Copy link
Member

jbaxleyiii commented Oct 7, 2016

Fixes #245

Docs PR apollographql/react-docs#70

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
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@jbaxleyiii jbaxleyiii merged commit 6554026 into master Oct 7, 2016

5 checks passed

./dist/index.min.js +269 bytes (+1.00%) → 26,929 bytes
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.289%
Details

@jbaxleyiii jbaxleyiii deleted the 245 branch Oct 7, 2016

@glasser

This comment has been minimized.

Copy link
Contributor

glasser commented Oct 7, 2016

Awesome!

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 12, 2016

The old API doesn't seem to work after this PR (in our case). Moreover, adding a deprecation warning would help us figure out we need to migrate. Maybe 0.5.8 should have been 0.6.0, too much feature changes at once: this broke our app.

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Oct 12, 2016

@rricard what is broken? The old API is still covered in testing https://github.com/apollostack/react-apollo/blob/master/test/react-web/client/graphql/queries-1.test.tsx#L321-L345

We aren't even planning on removing it as it allows for a different kind of skipping

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 12, 2016

I saw the test, but it seem to change the skipping behavior all at once: for instance the old test doesn't cover the case where I have two queries at once mutally skipping themselves. What I see are cancellations of the non-skipped query because the other query has been skipped.

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 12, 2016

I'll try to write a test for that. All I'm asking is maybe, while adding new APIs or refactors, don't just increment the patch number. Increment the minor version number instead. That way npm won't get the patch version right away, potentially breaking the app

@glasser

This comment has been minimized.

Copy link
Contributor

glasser commented Oct 12, 2016

Have you tried with 0.5.9? 0.5.9 fixes a bunch of skip related bugs, some
of which may have been new with 0.5.8.

On Oct 12, 2016 5:08 AM, "Robin Ricard" notifications@github.com wrote:

I'll try to write a test for that. All I'm asking is maybe, while adding
new APIs or refactors, don't just increment the patch number. Increment the
minor version number instead. That way npm won't get the patch version
right away, potentially breaking the app


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#253 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABBVK8NHNZBUdeEpuCj3sHfy8b5uQi0ks5qzM2ngaJpZM4KRZWk
.

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 12, 2016

We tried with 0.5.9 didn't work either. Bottom line is, with this kind of change: the safe path is to go with a new minor version...

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Oct 12, 2016

@rricard yeah, I hear you. Maybe this is something we should do for Apollo Client as well.

@glasser

This comment has been minimized.

Copy link
Contributor

glasser commented Oct 13, 2016

Admittedly, the "patch" and "minor" fields mean something a little different with 0.* semvers, but this makes a lot of sense.

@rricard That said, we didn't intend to break anything about the original skip feature other than fixing some blatant bugs. Can you file an issue for what broke in your app?

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 13, 2016

@helfer already talked with @stubailo about that, I think client is figured out, current refactor will go to an another minor number and/or a prerelease cycle (we have to figure out how to do that with npm though 😉!)

@glasser I know, I'm more trying to think about a threshold of refactor where we should start to think that something may break ... even if current tests won't catch it. What I'm saying is, that would just avoid the case where someone is tracking ^0.X.0 and a patch version would make fail a CI or a deployment, I think that increasing the minor where we're not quite completely sure would ensure we don't kill those. However, from now on and before the 1.0, I'll track exact patch versions.

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 13, 2016

Now, let's write some failing tests! 😉 ...and a fix! 🎉

@rricard

This comment has been minimized.

Copy link
Member

rricard commented Oct 13, 2016

See #271 for reproduction and bug tracking of the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment