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

Observable query refactor #211

Merged
merged 15 commits into from Oct 8, 2016

Conversation

Projects
None yet
3 participants
@tmeasday
Copy link
Contributor

tmeasday commented Sep 14, 2016

@jbaxleyiii this is just a first pass for you to look at, still a bit more to do (see XXXs in code and some notes I will leave now).

I welcome any comments you have at this stage though!

(Note that this branch runs with the add-observable-query-current-result branch of AC)

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Sep 14, 2016

Also some of the commits you may want to cherry pick, especially e2b9587

@@ -496,13 +496,16 @@ export interface UpdateQueryOptions {
}
export class ObservableQuery extends Observable<ApolloQueryResult> {
refetch: (variables?: any) => Promise<ApolloQueryResult>;
setVariables: (variables: any) => Promise<ApolloQueryResult>;

This comment has been minimized.

@tmeasday

tmeasday Sep 14, 2016

Author Contributor

I manually copied this over from apollo-client -- I'm not sure what the correct procedure is.

@@ -1209,7 +1232,7 @@ describe('queries', () => {
this.props.data.updateQuery();
done(new Error('should have thrown'))
} catch (e) {
expect(e).to.match(/Invariant Violation:/);
expect(e).to.match(/ObservableQuery with this id doesn't exist:/);

This comment has been minimized.

@tmeasday

tmeasday Sep 14, 2016

Author Contributor

There's no longer an invariant check here because it goes straight through to AC, but I think the error is OK? I'm not sure. We can definitely improve the error at AC's end anyway.

} else if (count === 1) {
expect(props.data.variables).to.deep.equal(variables2);
expect(props.data.variables).to.deep.equal(variables);

This comment has been minimized.

@tmeasday

tmeasday Sep 14, 2016

Author Contributor

fetchMore is not going to change data.variables anymore, but I think this is more correct, no? (the variables for fetchMore might be completely different to those for the original query).

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Sep 26, 2016

@jbaxleyiii I think this is more or less done as I was hoping now. When you have a moment to look at this could you take a look? I'll wait for your approval before merging the changes to AC and ultimately these into RA.

Also, you would have seen apollographql/apollo-client#707 which is relevant and something I'll look at soon.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Sep 26, 2016

@tmeasday 🎉 awesome! I'll take some time today to investigate it and look around!

@tmeasday tmeasday force-pushed the observableQuery-refactor branch from 90e5e3e to 16dece1 Sep 30, 2016

@tmeasday tmeasday changed the title Observable query refactor [WIP] Observable query refactor Sep 30, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor Author

tmeasday commented Sep 30, 2016

🎉 ready to go 🎉

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Oct 8, 2016

@tmeasday I totally missed your rebase! I'm super sorry! I'll rebase it after the recent changes!

James Baxley
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Oct 8, 2016

@tmeasday -9,170 bytes (-49.38%) → 18,570 bytes size reduction ftw!!

James Baxley added some commits Oct 8, 2016

James Baxley
James Baxley

@jbaxleyiii jbaxleyiii merged commit 246eca0 into master Oct 8, 2016

5 checks passed

./dist/index.min.js -9,170 bytes (-49.38%) → 18,570 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 decreased (-0.7%) to 95.34%
Details

@jbaxleyiii jbaxleyiii deleted the observableQuery-refactor branch Oct 8, 2016

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Oct 8, 2016

@tmeasday fantastic work!

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Oct 8, 2016

Wow this is crazy! Awesome.

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