Remove the dependencies on queries and mutations in the store from the data reducer #1845

shadaj · 2017-06-30T18:10:25Z

Depends on #1857

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

Awesome, this is a big improvement! Just a few suggestions.

stubailo · 2017-07-03T02:40:32Z

src/core/QueryManager.ts

@@ -1074,14 +1091,18 @@ export class QueryManager {

          const extraReducers = this.getExtraReducers();

+          const variablesObject = variables as Object;


That's odd - is there a mismatch between the type here and in the Redux action? I feel like we should fix that instead of casting somehow.

stubailo · 2017-07-03T02:41:47Z

src/core/QueryManager.ts

@@ -170,6 +174,8 @@ export class QueryManager {
  // updateQueries.
  private queryIdsByName: { [queryName: string]: string[] };

+  private lastRequestId: { [queryName: string]: number } = {};


I think you want queryId instead of queryName here (it doesn't actually matter what that's called but best to be accurate)

stubailo · 2017-07-03T02:47:03Z

src/actions.ts

  operationName: string | null;
  requestId: number;
+  lastRequestId: number;


I just checked, and I don't think we need this in the action at all. As far as I can tell, APOLLO_QUERY_RESULT actions are completely ignored if requestId < lastRequestId, so we can just not send them. This means adding this conditional before the action is dispatched.

stubailo · 2017-07-03T02:50:11Z

src/core/QueryManager.ts

-        updateQueries[queryId] = updateQueriesByName[queryName];
-      }));
-    }
+    const updateQueries: () => { [queryId: string]: QueryUpdate } = () => {


I'd call this something a bit more descriptive like generateUpdateQueriesInfo.

stubailo · 2017-07-03T02:50:31Z

src/actions.ts

@@ -88,6 +95,11 @@ export function isQueryStopAction(action: ApolloAction): action is QueryStopActi
  return action.type === 'APOLLO_QUERY_STOP';
 }

+export type QueryUpdate = {


I think this type could have a more descriptive name, or at least a comment describing what it's for.

stubailo · 2017-07-03T02:54:08Z

I'm not sure why the package lock changed - can we commit that in a separate PR rather than making it part of this one?

stubailo

Looks great, just add one comment and we're good to merge!

Don't forget to add to the change log.

stubailo · 2017-07-03T23:26:24Z

src/core/QueryManager.ts

-            fetchMoreForQueryId,
-            extraReducers,
-          });
+          if (requestId >= (this.lastRequestId[queryId] || 1)) {


This is kind of an odd boolean expression here. Can we add a comment explaining what it does?

stubailo · 2017-07-05T05:06:52Z

CHANGELOG.md

@@ -1,7 +1,7 @@
 # Change log

 ### vNEXT
-
+- Remove the dependency on the query and mutation store from the data reducer


Can you add the link to the PR as well? I also just noticed we didn't have that in a previous entry, but let's make sure to keep doing it so it's easy to find where the change was added.

stubailo · 2017-07-05T05:07:48Z

I think this can be released as 1.8? It changes the format of Redux actions which I feel is enough for a minor change.

Thoughts, @jbaxleyiii?

jbaxleyiii

@shadaj I left one comment to make sure we don't miss a possible throw, otherwise I'm good with it! Great work!

jbaxleyiii · 2017-07-05T16:03:25Z

src/data/store.ts

-            return;
-          }
-
+          const { query, reducer } = updateQueries[queryId];


@shadaj there isn't a chance that updateQueries[queryId] could return undefined right? If it did, this would throw

I don't think we have to worry about it returning undefined, since we are only using keys from iterating over Object.keys(updateQueries).

@shadaj that will only ensure that the key exists, not that the value is there :(

https://runkit.com/jbaxleyiii/595d29c0e60f6b0012bffe69

You could also something like this:
Object.keys(updateQueries).filter(id => queries[id]).forEach(....

jbaxleyiii · 2017-07-05T16:05:54Z

@stubailo I think we can release it as 1.8 but maybe lets put a larger note in the changelog for anyone relying on it with custom redux work?

…e data resolver

shadaj · 2017-07-05T20:38:06Z

Everything should be fixed now, but we should merge #1857 before we merge this PR.

jbaxleyiii · 2017-07-07T17:12:18Z

@shadaj @stubailo this will be in 1.8 👍

stubailo suggested changes Jul 3, 2017

View reviewed changes

shadaj changed the title ~~Remove the dependencies on queries in the store from the data resolver~~ Remove the dependencies on queries and mutations in the store from the data resolver Jul 3, 2017

stubailo approved these changes Jul 3, 2017

View reviewed changes

stubailo reviewed Jul 5, 2017

View reviewed changes

jbaxleyiii self-requested a review July 5, 2017 16:00

jbaxleyiii assigned shadaj Jul 5, 2017

jbaxleyiii approved these changes Jul 5, 2017

View reviewed changes

shadaj force-pushed the data-reducer-no-queries branch 2 times, most recently from 22f0612 to ce2649a Compare July 5, 2017 19:12

Remove the dependencies on queries and mutations in the store from th…

17fc938

…e data resolver

shadaj force-pushed the data-reducer-no-queries branch from ce2649a to 17fc938 Compare July 5, 2017 20:26

shadaj mentioned this pull request Jul 5, 2017

Remove mutation tracking from the store #1846

Merged

4 tasks

Merge branch 'master' into data-reducer-no-queries

e2bfded

Merge branch 'master' into data-reducer-no-queries

9465c5c

jbaxleyiii merged commit de7adfe into master Jul 7, 2017

jbaxleyiii deleted the data-reducer-no-queries branch July 7, 2017 17:19

stubailo changed the title ~~Remove the dependencies on queries and mutations in the store from the data resolver~~ Remove the dependencies on queries and mutations in the store from the data reducer Jul 7, 2017

stubailo mentioned this pull request Jul 7, 2017

Decouple GraphQL store with a generic API #1432

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 dependencies on queries and mutations in the store from the data reducer #1845

Remove the dependencies on queries and mutations in the store from the data reducer #1845

shadaj commented Jun 30, 2017 •

edited

Loading

stubailo left a comment

stubailo Jul 3, 2017

stubailo Jul 3, 2017

stubailo Jul 3, 2017

stubailo Jul 3, 2017

stubailo Jul 3, 2017

stubailo commented Jul 3, 2017

stubailo left a comment

stubailo Jul 3, 2017

stubailo Jul 5, 2017

stubailo commented Jul 5, 2017

jbaxleyiii left a comment

jbaxleyiii Jul 5, 2017

shadaj Jul 5, 2017

jbaxleyiii Jul 5, 2017

jbaxleyiii commented Jul 5, 2017

shadaj commented Jul 5, 2017

jbaxleyiii commented Jul 7, 2017

		@@ -1074,14 +1091,18 @@ export class QueryManager {

		const extraReducers = this.getExtraReducers();

		const variablesObject = variables as Object;

Remove the dependencies on queries and mutations in the store from the data reducer #1845

Remove the dependencies on queries and mutations in the store from the data reducer #1845

Conversation

shadaj commented Jun 30, 2017 • edited Loading

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

stubailo commented Jul 3, 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

stubailo commented Jul 5, 2017

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

jbaxleyiii commented Jul 5, 2017

shadaj commented Jul 5, 2017

jbaxleyiii commented Jul 7, 2017

shadaj commented Jun 30, 2017 •

edited

Loading