Remove the query store from Redux #1859

shadaj · 2017-07-05T23:35:25Z

Depends on #1846

This moves out the query store out from Redux into a separate class. Many now unused Redux actions are still dispatched for compatibility with applications that depend on those actions.

TODO:

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

stubailo · 2017-07-10T04:35:19Z

@shadaj I changed the target from master to no-mutations-store so this PR has only the changes from that.

stubailo

I have a few comments, but I also seem to recall we wanted to put all this stuff inside ObservableQuery. This can be a fine intermediate step I guess, but did we want to just go straight for that?

stubailo · 2017-07-10T04:36:13Z

src/core/QueryManager.ts

@@ -445,6 +447,12 @@ export class QueryManager {

    // Initialize query in store with unique requestId
    this.queryDocuments[queryId] = queryDoc;
+
+    this.queryStore.initQuery(queryId, queryString, queryDoc, shouldFetch, variables, fetchType === FetchType.poll,


Can we make this take keyword arguments? Lists of positional arguments with more than 2 items freak me out since it's so easy to get the order wrong.

stubailo · 2017-07-10T04:36:39Z

src/core/QueryManager.ts

+    this.queryStore.initQuery(queryId, queryString, queryDoc, shouldFetch, variables, fetchType === FetchType.poll,
+                              fetchType === FetchType.refetch, metadata, fetchMoreForQueryId);
+
+    this.broadcastQueries();


Should this go after the dispatch?

stubailo · 2017-07-10T04:40:53Z

src/core/QueryManager.ts

+
+            this.broadcastQueries = orig;
+
+            const { reducerError } = this.getApolloState();


Oh man, this sucks.

stubailo · 2017-07-10T04:41:53Z

src/core/QueryManager.ts

@@ -1110,6 +1129,9 @@ export class QueryManager {

          // default the lastRequestId to 1
          if (requestId >= (this.lastRequestId[queryId] || 1)) {
+            const orig = this.broadcastQueries;
+            this.broadcastQueries = () => undefined;


I really want to find some way to not have to turn off query broadcasting in this way, like if we could maybe update the query status first?

However, if we really have to I'd prefer a flag like disableBroadcasting = true rather than overwriting the function like this.

stubailo · 2017-07-10T04:42:31Z

src/queries/store.ts


-    const previousQuery = previousState[action.queryId];
+  public initQuery(queryId: string, queryString: string, document: DocumentNode, storePreviousVariables: boolean,
+                   variables: Object, isPoll: boolean, isRefetch: boolean, metadata: any, fetchMoreForQueryId: string | undefined) {


Def switch this to a single object argument.

shadaj · 2017-07-10T17:49:28Z

I think this can be put in as an intermediate change because it maintains the shape of the original Redux store, which eases the transition for people who were directly reading from the store in Redux before.

stubailo · 2017-07-11T00:27:32Z

Any reason the tests aren't passing?

shadaj · 2017-07-11T00:30:36Z

Looks like the bundle size checker is saying that the built bundle is too large.

stubailo · 2017-07-12T18:20:21Z

src/core/QueryManager.ts

@@ -1110,6 +1139,9 @@ export class QueryManager {

          // default the lastRequestId to 1
          if (requestId >= (this.lastRequestId[queryId] || 1)) {
+            const orig = this.broadcastQueries;


This line is not used anymore

jbaxleyiii

@shadaj a few minor changes and questions here. This is looking great! Excited to alpha it asap!

jbaxleyiii · 2017-07-13T18:42:46Z

src/core/QueryManager.ts

      isPoll: fetchType === FetchType.poll,
      isRefetch: fetchType === FetchType.refetch,
-      fetchMoreForQueryId,
-      metadata,
+      metadata, fetchMoreForQueryId,


can we new line this?

jbaxleyiii · 2017-07-13T18:44:19Z

src/mutations/store.ts

@@ -1,12 +1,12 @@
 export class MutationStore {
-  private store: {[mutationId: string]: MutationStoreValue} = {};
+  public _store: {[mutationId: string]: MutationStoreValue} = {};


If this is public, why the underscore prefix? Would it be better to have this behind a getter?

jbaxleyiii · 2017-07-13T18:44:50Z

src/queries/store.ts

-  selectionSet: SelectionSetNode;
-}
+export class QueryStore {
+  public _store: {[queryId: string]: QueryStoreValue} = {};


Same question as the mutation store access level

jbaxleyiii · 2017-07-13T18:45:09Z

src/queries/store.ts


-    const previousQuery = previousState[action.queryId];
+  public initQuery(query: {queryId: string, queryString: string, document: DocumentNode, storePreviousVariables: boolean,


*nit: can we reformat this to make it more legible?

jbaxleyiii · 2017-07-13T18:47:07Z

test/tests.ts

 process.env.NODE_ENV = 'test';
+QueryManager.EMIT_REDUX_ACTIONS = false;



@shadaj can we add a test to make sure that actions are still emitted when this is true? I'd like to make sure both cases work

shadaj · 2017-07-13T19:23:07Z

@jbaxleyiii everything should be fixed now!

* Remove mutation tracking from the store * Remove the query store from Redux (#1859)

shadaj self-assigned this Jul 5, 2017

shadaj force-pushed the no-query-store branch 2 times, most recently from 31def68 to 0d91982 Compare July 7, 2017 17:37

stubailo mentioned this pull request Jul 7, 2017

Decouple GraphQL store with a generic API #1432

Closed

stubailo changed the base branch from master to no-mutations-store July 10, 2017 04:34

stubailo suggested changes Jul 10, 2017

View reviewed changes

shadaj force-pushed the no-mutations-store branch from 3e81ea4 to fd1017d Compare July 10, 2017 17:38

shadaj force-pushed the no-query-store branch from 0d91982 to 45c6cf3 Compare July 10, 2017 17:40

shadaj force-pushed the no-query-store branch from c823fed to 4f1fee8 Compare July 10, 2017 22:32

shadaj force-pushed the no-mutations-store branch from fd1017d to db5d7a2 Compare July 12, 2017 17:51

shadaj force-pushed the no-query-store branch from 4f1fee8 to 8e8e82f Compare July 12, 2017 17:52

stubailo suggested changes Jul 12, 2017

View reviewed changes

shadaj force-pushed the no-query-store branch 5 times, most recently from 4127a4b to 1756ab3 Compare July 12, 2017 23:16

jbaxleyiii suggested changes Jul 13, 2017

View reviewed changes

jbaxleyiii added the waiting-response label Jul 13, 2017

shadaj removed the waiting-response label Jul 13, 2017

shadaj force-pushed the no-mutations-store branch 2 times, most recently from a8b1675 to 1e6e7e2 Compare July 13, 2017 19:25

shadaj force-pushed the no-query-store branch from 7945b86 to acaa146 Compare July 13, 2017 19:26

shadaj force-pushed the no-mutations-store branch from 1e6e7e2 to e0a93dc Compare July 13, 2017 21:22

shadaj force-pushed the no-query-store branch from acaa146 to d96c0c4 Compare July 13, 2017 21:22

shadaj force-pushed the no-mutations-store branch from e0a93dc to 05f1aaa Compare July 13, 2017 21:27

Remove the query store from Redux

cfb45df

shadaj force-pushed the no-query-store branch from d96c0c4 to cfb45df Compare July 13, 2017 21:32

jbaxleyiii approved these changes Jul 13, 2017

View reviewed changes

jbaxleyiii merged commit aa43297 into no-mutations-store Jul 13, 2017

jbaxleyiii deleted the no-query-store branch July 13, 2017 23:05

jbaxleyiii pushed a commit that referenced this pull request Jul 13, 2017

Remove mutation tracking from the store (#1846)

162e5cc

* Remove mutation tracking from the store * Remove the query store from Redux (#1859)

shadaj added a commit that referenced this pull request Jul 13, 2017

Remove the query store from Redux (#1859)

4b05586

pcorey mentioned this pull request Jul 19, 2017

How to achieve offline support? #424

Closed

steven-js mentioned this pull request Jul 13, 2021

useMutation - Safari does not refetchQueries, fetchPolicy only supports 'no-cache' #8494

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the query store from Redux #1859

Remove the query store from Redux #1859

shadaj commented Jul 5, 2017 •

edited

Loading

stubailo commented Jul 10, 2017

stubailo left a comment

stubailo Jul 10, 2017

stubailo Jul 10, 2017

stubailo Jul 10, 2017

stubailo Jul 10, 2017

stubailo Jul 10, 2017

shadaj commented Jul 10, 2017

stubailo commented Jul 11, 2017

shadaj commented Jul 11, 2017

stubailo Jul 12, 2017

jbaxleyiii left a comment

jbaxleyiii Jul 13, 2017

jbaxleyiii Jul 13, 2017

jbaxleyiii Jul 13, 2017

jbaxleyiii Jul 13, 2017

jbaxleyiii Jul 13, 2017

shadaj commented Jul 13, 2017


		this.broadcastQueries = orig;

		const { reducerError } = this.getApolloState();


		const previousQuery = previousState[action.queryId];
		public initQuery(query: {queryId: string, queryString: string, document: DocumentNode, storePreviousVariables: boolean,

		process.env.NODE_ENV = 'test';
		QueryManager.EMIT_REDUX_ACTIONS = false;

Remove the query store from Redux #1859

Remove the query store from Redux #1859

Conversation

shadaj commented Jul 5, 2017 • edited Loading

stubailo commented Jul 10, 2017

stubailo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadaj commented Jul 10, 2017

stubailo commented Jul 11, 2017

shadaj commented Jul 11, 2017

Choose a reason for hiding this comment

jbaxleyiii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shadaj commented Jul 13, 2017

shadaj commented Jul 5, 2017 •

edited

Loading