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

Fix crash in unsubscribeFromQuery #260

Merged
merged 1 commit into from Oct 11, 2016

Conversation

Projects
None yet
3 participants
@glasser
Copy link
Contributor

glasser commented Oct 11, 2016

Also delete querySubscription when there is no subscription, since that
seems reasonable.

See #255.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 11, 2016

@glasser do you know what the circumstance that leads to this bug is? Seems likely to be either:

a) component receives new props before it has mounted
b) component unmounts before it has mounted.

Neither seems like valid react lifecycle but I'm not initimately aware of the rules surrounding it. Can we figure out which it is so we can add a test?

@glasser glasser force-pushed the glasser/unsubscribe-crash branch 2 times, most recently from 1036a23 to baa329f Oct 11, 2016

@tmeasday tmeasday force-pushed the glasser/unsubscribe-crash branch 2 times, most recently from 34032af to c2af14a Oct 11, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 11, 2016

@glasser I ended up getting rid of promise code, not really needed in jasmine (I had my mocha hat on).

@tmeasday tmeasday assigned glasser and unassigned tmeasday Oct 11, 2016


networkInterface.query = (request) => {
console.log('failing')
fail(new Error('query ran even though skip present'));

This comment has been minimized.

@glasser

glasser Oct 11, 2016

Author Contributor

I can't figure out what fail is. It's not in https://facebook.github.io/jest/docs/api.html or defined in the file. But it does seem to work (I tested with the change reverted...)

This comment has been minimized.

@tmeasday

tmeasday Oct 11, 2016

Contributor

I guess fail is a Jasmine thing. Jest's docs seem a bit incomplete.

@@ -495,6 +614,44 @@ describe('queries', () => {
renderer.create(<ProviderMock client={client}><ChangingProps /></ProviderMock>);
});


fit('allows you to unmount a skipped query', (done) => {

This comment has been minimized.

@glasser

glasser Oct 11, 2016

Author Contributor

Changing to it

const oldQuery = networkInterface.query;

networkInterface.query = (request) => {
console.log('failing')

This comment has been minimized.

@glasser

glasser Oct 11, 2016

Author Contributor

Removing this line

renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
});

it('allows you to skip then unskip a query with opts syntax', (done) => {

This comment has been minimized.

@glasser

glasser Oct 11, 2016

Author Contributor

This one wasn't actually failing (without the code change). Or maybe it was failing if you only reverted one bit of the code change but not the whole commit.

I've made it longer and more detailed, but then it doesn't actually pass any more: looks like I can't convince it to run the query again. Maybe that's due to some caching and that's a feature? I guess @tmeasday is asleep now; @jbaxleyiii want to take a look at my version of this test and help me figure out how to get it to fail? The goal is to trigger a now-fixed bug where the check on opts.skip in subscribeToQuery doesn't delete querySubscription and thus never re-subscribes when unskipped.

networkInterface.query = (request) => {
console.log('failing')
fail(new Error('query ran even though skip present'));
return oldQuery(request);

This comment has been minimized.

@glasser

glasser Oct 11, 2016

Author Contributor

Lack of preserving this caused problems when I copied this into other test. fixed.

@glasser glasser force-pushed the glasser/unsubscribe-crash branch 2 times, most recently from a364db1 to 6845cb8 Oct 11, 2016

Fix bugs around unsubscribe and skipping
See #255.

@glasser and I found a few bugs around the (old and new) skip options,
and wrote some tests to confirm the fixes and stop regressions

- didn't delete querySubscription in .unsubscribeFromQuery()
  - this mean if you skipped and then unskipped, it would break

- not checking for querySubscription before .unsubscribe on unmounting
  - if you skip then unmount, you have problem

- check for non-skip -> skip in willReceiveProps didn't return in the remain skipping case
  - mean "non skipping" code ran when remaining skipping.

- Make the default data have an "errors" null field rather than "error"
  since that matches what is done elsewhere.

- Also make sure to consistently set it to null rather than undefined.

- Be a little more consistent about how we initialize and re-initialize
  this.data; for example, if we use the deprecated skip option to change
  an unskipped query to a skipped query, include "error: null" as well,
  and if we are switching from unskip to skip to unskip, make sure to
  re-initialize the full data object.

@glasser glasser force-pushed the glasser/unsubscribe-crash branch from 6845cb8 to 8ee1a1f Oct 11, 2016

@glasser glasser assigned tmeasday and jbaxleyiii and unassigned glasser Oct 11, 2016

@glasser

This comment has been minimized.

Copy link
Contributor Author

glasser commented Oct 11, 2016

OK, I put a little more effort into this and got all the new tests to pass (and to fail without the graphql.tsx changes). This involved a few more changes to the real code, all of which I think are bug fixes, but I'm looking at all this code for the first time and I might be wrong! I'd love another round of review from @jbaxleyiii or @tmeasday or anyone familiar with the code!

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 11, 2016

I wonder if rather than adding and removing the observable query fields from this.data we should instead just blindly pass skippedQueryData to the component at render time (in the skip case)?

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 11, 2016

Or do we need the other parts of initializeData() (the currentResult part) also when skip->unskipping?

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 11, 2016

Or maybe we should just copy the properties over at render time and not attach them to data in the first place. I might do that.

In any case, I am good with these changes. Merging.

@tmeasday tmeasday merged commit d6e110b into master Oct 11, 2016

5 checks passed

./dist/index.min.js +126 bytes (+0.67%) → 18,696 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 increased (+1.2%) to 96.558%
Details
@glasser

This comment has been minimized.

Copy link
Contributor Author

glasser commented Oct 11, 2016

The main thing i was going for by inserting the initializeData call in subscribeToQuery was to ensure that when you "unskip" that you end up with loading: true set even though it was false previously because of the skipping. I don't 100% understand every bit of this code so my intuition may be off.

I think the idea of handling skipping directly in render makes sense.

glasser added a commit that referenced this pull request Oct 11, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 12, 2016

I think we can go a bit further and actually not cache this.data at all. It should make things simpler and more declarative (and delegate more work to AC): #264

@calebmer calebmer deleted the glasser/unsubscribe-crash branch Mar 1, 2017

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