Skip to content
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

StoreFetchMiddleware #376

Closed
wants to merge 2 commits into from

Conversation

nevir
Copy link
Contributor

@nevir nevir commented Jul 10, 2016

StoreFetchMiddleware allows the caller to rewrite fetches from the store (for example, to redirect a read to another node)

This also includes a reference implementation to cover the case outlined by #332 (and I suspect the primary use of this middleware):

import ApolloClient, { cachedFetchById } from 'apollo-client';
const client = new ApolloClient({
  dataIdFromObject: value => value.id,
  storeFetchMiddleware: cachedFetchById,
});

await client.query({
 query: gql`
    query fetchAll {
      tasks { id, name }
    }
  `,
});

// abc123 is fetched from cache, if fetchAll returned it!
await client.query({
 query: gql`
    query fetchOne($id: ID!) {
      task(id: $id) { id, name }
    }
  `,
  variables: { id: "abc123" },
});

Also, FYI, Mocha is promise-aware - just return the promise from a test, and it'll take care of the rest! No need to use done

nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 11, 2016
Prompted by apollographql#376's brittleness; this helps keep common state/configuration around when performing a query.

---

Also, as part of this, I noticed there are two seemingly unused functions: `diffQueryAgainstStore`, `diffFragmentAgainstStore`
@nevir nevir force-pushed the store-fetch-middleware branch 3 times, most recently from 33d6f9d to a289de1 Compare July 11, 2016 04:14
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 11, 2016
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 14, 2016
Prompted by apollographql#376's brittleness; this helps keep common state/configuration around when performing a query.

---

Also, as part of this, I noticed there are two seemingly unused functions: `diffQueryAgainstStore`, `diffFragmentAgainstStore`
@nevir nevir force-pushed the store-fetch-middleware branch from a289de1 to a5db83c Compare July 14, 2016 22:16
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 14, 2016
Prompted by apollographql#376's brittleness; this helps keep common state/configuration around when performing a query.

---

Also, as part of this, I noticed there are two seemingly unused functions: `diffQueryAgainstStore`, `diffFragmentAgainstStore`
@nevir nevir force-pushed the store-fetch-middleware branch 2 times, most recently from badf40c to d91de58 Compare July 14, 2016 22:53
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 15, 2016
@nevir nevir mentioned this pull request Jul 19, 2016
@nevir nevir changed the title Introducing StoreFetchMiddleware StoreFetchMiddleware Jul 21, 2016
@nevir nevir force-pushed the store-fetch-middleware branch from d91de58 to 2aeabc9 Compare July 21, 2016 21:34
@nevir
Copy link
Contributor Author

nevir commented Jul 21, 2016

Alrighty, rebased & it now handles the new IdValue structure for cachedFetchById

@nevir nevir force-pushed the store-fetch-middleware branch from 2aeabc9 to ab54ca2 Compare July 21, 2016 22:25
nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 21, 2016
@nevir nevir force-pushed the store-fetch-middleware branch from ab54ca2 to cf76a22 Compare July 22, 2016 19:17
@CharlesMangwa
Copy link

Hi guys! I'm currently working on a React Native where we use Apollo Client and StoreFetchMiddleware is exactly what we need actually 😱 !! Are you planning to work on it (maybe merge it?) anytime soon?

@nevir nevir force-pushed the store-fetch-middleware branch 2 times, most recently from ebd8a49 to e3b18bf Compare July 25, 2016 17:10
@LeoLeBras
Copy link

+1

@nevir nevir force-pushed the store-fetch-middleware branch from e3b18bf to a31251d Compare July 26, 2016 16:44
@nevir
Copy link
Contributor Author

nevir commented Aug 7, 2016

Curious what you guys think about this PR?

@igrayson
Copy link
Contributor

any update?

@nevir nevir force-pushed the store-fetch-middleware branch from d64335f to 7858f91 Compare August 16, 2016 20:09
This makes it quite a bit easier to pass additional configuration to the data/* functions
@nevir nevir force-pushed the store-fetch-middleware branch from 7858f91 to a7aa785 Compare August 16, 2016 20:10
By allowing users to rewrite data lookups from the store, they can satisfy behavior like apollographql#332
@nevir nevir force-pushed the store-fetch-middleware branch from a7aa785 to d1ecb7e Compare August 16, 2016 20:10
nevir added a commit to convoyinc/apollo-client that referenced this pull request Aug 16, 2016
@nevir
Copy link
Contributor Author

nevir commented Aug 16, 2016

The test failure is node v4 only with:

=============================== FileSize summary ===============================
The total size of index.min.js is 139.77 kB.
The total gzipped size of index.min.js is 37.16 kB.
================================================================================
The total gzipped size of index.min.js exceeds 37 kB.

@igrayson
Copy link
Contributor

Could anyone provide feedback?

@LeoLeBras
Copy link

LeoLeBras commented Aug 27, 2016

@stubailo Can you please provide any estimations on when this pull request will be merged ?

@deoqc
Copy link

deoqc commented Sep 12, 2016

Hey @nevir @stubailo, what is the state of this pr?

I have some time and need this feature, so would like to help if I can.

@stubailo
Copy link
Contributor

Hey, sorry I've been a blocker on this PR for a while. I think in order to avoid people manipulating the query AST directly, we should implement this feature after #615, which will let us switch the internal execution implementation to use https://github.com/apollostack/graphql-anywhere

This will allow the store fetch middleware to instead be replaced with custom resolvers on the client, which is much more similar to how GraphQL works on the server, rather than using AST manipulation.

We're hoping to get this refactor done in the next week or two, so perhaps we can wait until then? However if it's really urgent we can add a more hacky solution first. I'd suggest that the hacky solution should not live in the execution layer, but instead somewhere in query manager, where the query can be transformed before being passed into diffQueryAgainstStore.

@deoqc
Copy link

deoqc commented Sep 12, 2016

Waiting 2 weeks (or a bit more) is just fine for me... I was worried 'cause this is an essential feature.

@stubailo
Copy link
Contributor

Yeah, I agree it's important to be able to use data from the cache, even if you are running a different query. I think it will be way easier after a refactor though, which might also come with a bonus of using any client-side data from GraphQL.

@nevir
Copy link
Contributor Author

nevir commented Sep 13, 2016

Yeah, local resolvers would be a much better solution

@paynecodes
Copy link

@stubailo I thought @nevir was on to something useful with this PR. I understand that the implementation of something like this will change with #615, though. Is there something else planned for covering the use cases here?

@stubailo
Copy link
Contributor

Yeah, if you look at the conversation above we concluded that this feature can be implemented in a much better way after a refactor. Then, you don't have to manipulate the AST, instead you'll be able to define custom client-side resolvers.

@paynecodes
Copy link

Gotcha. I saw that. I presumed this PR would reworked accordingly. It would be better to submit an entirely new PR. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants