This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Fix crash in unsubscribeFromQuery #260

Merged

tmeasday merged 1 commit into master from glasser/unsubscribe-crash

Oct 11, 2016

Member

glasser commented Oct 11, 2016 •

edited

Loading

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

See #255.

glasser assigned tmeasday

glasser force-pushed the glasser/unsubscribe-crash branch from 955eae7 to de8899c Compare

October 11, 2016 00:44

glasser mentioned this pull request

TypeError: Cannot read property 'variables' of undefined #255

Closed

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 force-pushed the glasser/unsubscribe-crash branch 2 times, most recently from 1036a23 to baa329f Compare

October 11, 2016 01:41

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

October 11, 2016 04:30

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 assigned glasser and unassigned tmeasday

glasser commented

View reviewed changes

test/react-web/client/graphql/queries-1.test.tsx

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

Member Author

glasser Oct 11, 2016 •

edited

Loading

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...)

Contributor

tmeasday Oct 11, 2016

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

test/react-web/client/graphql/queries-1.test.tsx

@@ @@ -495,6 +614,44 @@ describe('queries', () => { @@
                   renderer.create(<ProviderMock client={client}><ChangingProps /></ProviderMock>);
                 });
+                fit('allows you to unmount a skipped query', (done) => {

Member Author

glasser Oct 11, 2016 •

edited

Loading

Changing to it

test/react-web/client/graphql/queries-1.test.tsx

+                  const oldQuery = networkInterface.query;
+                  networkInterface.query = (request) => {
+                    console.log('failing')

Member Author

glasser Oct 11, 2016

Removing this line

test/react-web/client/graphql/queries-1.test.tsx

+                  renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
+                });
+                it('allows you to skip then unskip a query with opts syntax', (done) => {

Member Author

glasser Oct 11, 2016 •

edited

Loading

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.

test/react-web/client/graphql/queries-1.test.tsx

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

Member Author

glasser Oct 11, 2016

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

glasser force-pushed the glasser/unsubscribe-crash branch 2 times, most recently from a364db1 to 6845cb8 Compare

October 11, 2016 16:40


          Fix bugs around unsubscribe and skipping

8ee1a1f

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 force-pushed the glasser/unsubscribe-crash branch from 6845cb8 to 8ee1a1f Compare

October 11, 2016 17:43

glasser assigned tmeasday and jbaxleyiii and unassigned glasser

Member 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!

Contributor

tmeasday commented Oct 11, 2016 •

edited

Loading

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)?

Contributor

tmeasday commented Oct 11, 2016

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

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 merged commit d6e110b into master

Member 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


          changelog: tweak #260 description

a765e7c

tmeasday mentioned this pull request

Refactor to calculate this.data at render time #264

Closed

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 deleted the glasser/unsubscribe-crash branch

March 1, 2017 18:28

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

Labels

None yet