New skip method

[jbaxleyiii]

Fixes #245

Docs PR https://github.com/apollostack/react-docs/pull/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

[glasser]

Awesome!

[rricard]

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]

@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]

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]

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]

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" [email protected] 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]

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]

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

[glasser]

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]

@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]

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

[rricard]

See #271 for reproduction and bug tracking of the issue