Provide shouldInvalidatePreviousData prop

[jcready]

Fixes #2202

Checklist:

• If this PR is a new feature, please 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
• If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

[apollo-cla]

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

[hwillson]

Thanks for this PR @jcready! I'm wondering if we're over-complicating things here a bit. What if we just added something like a clearPreviousDataOnLoad prop. When it's true, we make sure previousData is emptied. This would simplify the RA codebase, and if an application really only wants to disable previousData based on previous / current variable differences, they can track variables and do the comparison outside of the Query component, and just pass the boolean result in via clearPreviousDataOnLoad.

[hwillson]

@jcready Can you explain why this is necessary?

[jcready]

I didn't want to re-write all the tests that depend on this functionality and it does seem like first is typically only used for pagination which means you generally wouldn't want to invalidate the previous data if it changed.

[jcready]

@hwillson clearPreviousDataOnLoad makes it sound like you would only clear the previous data once the network call finished. I think many people want the previous data cleared the moment their variables change. Additionally I know that many people wanted this to be the default behavior (e.g. pagination should be the exception).

[...] if an application really only wants to disable previousData based on previous / current variable differences, they can track variables and do the comparison outside of the Query component, and just pass the boolean result in via clearPreviousDataOnLoad.

To me that sounds like it would make things more difficult for the consumer since they would then have to keep track of the previous variables themselves. The Query component's componentWillReceiveProps already has access to both and can easily provide them to a passed in function.

[hwillson]

Thanks for the feedback @jcready. Regarding making this the default behavior, I agree, but we're kinda of stuck with things the way they are now (due to backwards compatibility issues). You touched on part of the bigger issue when you mentioned componentWillReceiveProps - we're in the process of moving away from React lifecycle methods in react-apollo 3.0. Another part of the issue I see here is that people will likely want similar behavior when they change other things, like changing the query for example. Hmmm ... thinking ...

[Titozzz]

@hwillson This is still an issue, and after the 3.1 release, it is not fixed. By the way, you might want to reopen #2202.
If you want a CodeSandbox reproducing this: https://codesandbox.io/s/context-vs-queries-f2079.

Also see: #3572

[iamswain25]

is it getting merged?