Fragment matcher #1483

helfer · 2017-03-23T22:21:46Z

Fixes #1337 #1363 #1379 #1113 #857 #771 #1297 #1273 #961 #933

Not 100% sure all of these issues will be fixed, but it should fix a good number of them.

TODO:

If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
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
Add your name and email to the AUTHORS file (optional)
If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

This is a failing test to demonstrate issue #1363.

stubailo

A few requests. There are also some comments left in from debugging.

stubailo · 2017-03-25T07:59:24Z

src/data/fragmentMatcher.ts

+  isTest,
+} from '../util/environment';
+
+export interface FragmentMatcherInstance {


This name is confusing because I usually thing Instance means not the interface. Perhaps FragmentMatcher or FragmentMatcherInterface would be a better name.

Yeah, I'll probably go for interface then, because there's already a FragmentMatcher in graphql-anywhere, and that one's a function.

stubailo · 2017-03-25T08:00:17Z

src/core/QueryManager.ts

+  // This function runs fetchQuery without initializing the fragment matcher.
+  // We always want to initialize the fragment matcher, so this function should not be accessible outside.
+  // The only place we call this function from is within fetchQuery after initializing the fragment matcher.
+  private fetchQueryWithoutInit<T>(


I'd call this fetchQueryAfterFragmentMatcherInit to be really explicit that you should only call it in that circumstance.

stubailo · 2017-03-25T08:02:08Z

src/data/fragmentMatcher.ts

+      throw new Error('FragmentMatcher.match() was called before FragmentMatcher.init()');
+    }
+
+    // invariant(isIdValue(idValue), 'boo hoo, I am sad');


Should be deleted

stubailo · 2017-03-25T08:03:57Z

src/data/fragmentMatcher.ts

+      return;
+    })
+    .catch( (err: any) => {
+      throw err;


There's definitely a possibility of a bad outcome here if we couldn't fetch the introspection result. I think a good option could be to set this.readyPromise = null so that we try to init again on the next query.

Yeah, probably worth a try 👍

stubailo · 2017-03-25T08:07:28Z

src/data/fragmentMatcher.ts

+  /**
+   * The init method has to get called before the match function can be used.
+   */
+  public init(queryManager: QueryManager): Promise<void> {


This is a bit nitpicky, but I think a name like ensureReady would be better because that would indicate that this function is called every time, not just once to initialize.

yup, agreed. The function evolved quite a bit, and it's no longer just an init function.

stubailo · 2017-03-25T08:11:15Z

src/data/fragmentMatcher.ts

+export interface FragmentMatcherInstance {
+  canBypassInit: (query: DocumentNode) => boolean;
+  init(queryManager?: QueryManager): Promise<void>;
+  match(idValue: IdValue, typeCondition: string, context: ReadStoreContext): boolean;


Should this take an obj instead of an idValue? Seems like both implementations just do obj = context.store[idValue.id] as the first statement. I can see that we still want context so we can set returnPartialData.

Yeah, I thought about that for a second, but then decided against it, because we might have a store in the future where the typename is stored in the path or in the id, in which case we could theoretically save the lookup. It also saves us from having to update graphql-anywhere or wrapping the fragmentMatcher function before passing it in. I think it's a little bit simpler this way, so we should keep it.

Sounds good. This is a small enough piece that if we change the store format or anything else later, rewriting this will be the least of our worries.

helfer · 2017-03-25T08:27:38Z

It's worth noting that client.readFragment and client.readQuery might break if the FragmentMatcher is not initialized. If they are called before the first query returns, the user has to ensure that the fragment matcher has been initialized if it needs to be.

stubailo · 2017-03-25T08:40:26Z

Yeah I guess there's no way to get around that if store reading is synchronous.

csillag · 2017-03-25T17:30:52Z

Hurray!

Where will be the next RC release?

helfer · 2017-03-25T17:36:43Z

@csillag I'll release it in a couple of minutes, but I won't tag it as latest yet, so you'll have to install that specific version or @next to get it.

To use the introspection fragment matcher for now, you'll also need to specifically provide it:

import ApolloClient, { IntrospectionFragmentMatcher } from 'apollo-client';

const client = new ApolloClient({
  fragmentMatcher: new IntrospectionFragmentMatcher(),
});

I'm doing this because I'd like to make sure that there will not be any unintended consequences of having to fetch (or pre-populate) some schema information before enabling it for everyone by default.

csillag · 2017-03-25T17:40:57Z

Awesome. I will take it for a spin on Monday.

helfer · 2017-03-25T17:51:29Z

cc @fubhy @mdebbar @bgentry @TSMMark @thebigredgeek I've merged a solution for fragment matching.

It would be great if you could test it by apollo-client@1.0.0-rc.7 and explicitly using the IntrospectionFragmentMatcher as per the instructions above. Once we've confirmed that it works for your apps, we'll flip the switch and roll it out for everyone 🎉

bgentry · 2017-03-25T20:24:56Z

reposting from slack:

@helfer AFAICT the Interface type stuff is fixed 🎉

Unfortunately the upgrade breaks some of my tests around error handling of 4xx server responses. I thought that my observer's error() wasn't getting called anymore in that scenario, but it's slightly more complicated than that.

I manipulated a cookie on my app to achieve the same effect outside of tests, and the result is that my actual app is broken by this change.

The reason is due to the fact that this change adds the following introspection query, which runs before any of my app's actual queries:

{
  __schema {
    types {
      kind
      name
      possibleTypes {
        name
      }
    }
  }
}

My tests assume that the first request that's made is one to load the current user, and the server just always returns a 401 in these tests (as it would IRL when an auth token is invalid for some reason). Thus the introspection query fails on a 401 status.

Likewise, my actual app (outside of tests) tries to load the current user immediately to check if auth is valid, and if not will invalidate the session and cause a login. But my current user query is never even running, presumably due to an error received on this introspection query.

I don't think this is viable because it requires the graphql endpoint to be publicly accessible with no authentication requirement, at least for the sake of this introspection query. My server's graphql endpoint is only available with a valid session, and I don't think that's a use case that can safely be ignored.

stubailo · 2017-03-25T21:15:16Z

@bgentry would adding a build step to inline that type information on startup work for you? Do you have the ability to run introspection locally from your development environment?

helfer · 2017-03-26T23:45:24Z

@bgentry I don't think there's any way of doing this without schema information. Having schema information requires that you either

A) Include the schema at build time (for example by importing, or by doing an additional build step)
B) Fetch the schema at runtime

For option B importing is only reasonable if your schema is already in JS, which isn't the case for everyone. It would also incur a bundle size penalty. It's pretty easy to implement such a fragment matcher with the interface available today, so if that's the option you want to go for, I'm happy to assist.
Option B could still be optimized such that queries can still be executed without the matcher being ready as long as the matcher isn't needed (store is empty or queries don't contain fragments, etc.). This solution would be quite a bit more complex however, which is why I'd rather not go too far down that path.

One of our most important goals with Apollo is that it works out of the box and doesn't require a build step or tooling for the most common use-cases.

@bgentry: just so I understand. How is it that your current user query can succeed but not the introspection query? Or are you using the fact that the currentUser query fails to set in motion some kind of login process? If so, could you just use the fact that the introspectionQuery fails for doing the same thing?

helfer · 2017-03-26T23:47:08Z

@TSMMark yes, it should work out of the box. Are you passing addTypename: false to Apollo by any chance?

stubailo · 2017-03-26T23:57:13Z

@bgentry how about if you could do option (2), but you had a script that generated the necessary data? So basically:

Generate a .js file using a very simple script that has the relevant schema information
Import that js file into your frontend code just like anything else, so it should work fine with the Ember build

In this setup, you would need re-run the script from (1) when you modify the interfaces or unions to your schema.

I'd much rather embed my whole schema in the frontend app if necessary than deal with a more tangled bootstrapping process

How does this compare to the process above for you?

TSMMark · 2017-03-27T00:04:03Z

Whoops, my bad @helfer. We had a custom dataIdFromObject which was throwing that.

const dataIdFromObject = (result) => {
  if (!result.__typename) {
    throw new Error('GraphQL response missing __typename: ' + JSON.stringify(result))
  }

I guess dataIdFromObject is applied to objects in the response of the schema query too. Intended? Seems a little odd that any part of the schema would need a dataId, no?

Edit: It appears that our dataIdFromObject was not needed so I removed it and now everything works great, and the new fragment matching seems to have done the trick. Thanks so much!

bgentry · 2017-03-27T00:57:43Z

I don't think there's any way of doing this without schema information.

Yep, totally get that!

One of our most important goals with Apollo is that it works out of the box and doesn't require a build step or tooling for the most common use-cases.

I think this is an important goal and it's one that I share.

@bgentry: just so I understand. How is it that your current user query can succeed but not the introspection query? Or are you using the fact that the currentUser query fails to set in motion some kind of login process? If so, could you just use the fact that the introspectionQuery fails for doing the same thing?

I am using the fact that the currentUser query fails (specifically with a 401) to determine that the session should be invalidated. Both the currentUser query and the introspection query fail in the same way, however afaict I have no way to know about the failed introspection query. None of my error handlers or promise catches were being called when I tested this out (though I could see that the Apollo query manager was receiving the error, that's as far as I got w/ debugging).

As far as bundling my schema, I am indeed already doing that for usage in my test suite, so it wouldn't be much different to put it in a place where the app itself could use it too.

Since Apollo Client never used the schema before this fragment matcher was added, am I correct to assume that there isn't a documented way to provide a schema? Would I need to implement a custom FragmentMatcherInterface, and if so, how much customization would be required? Since I'm sure I won't be the only one needing to do this, it's probably worthwhile to make sure it's easy to do and documented (the latter I can maybe help with).

stubailo · 2017-03-27T01:03:29Z

@bgentry the current design allows you to pass a static object with the data in the constructor: https://github.com/apollographql/apollo-client/pull/1483/files#diff-2223acc12993ca0745c660713b0c4f78R40

booboothefool · 2017-03-27T03:45:28Z

Hi, I arrived at this issue from #1436 and wanted to try this out. Upgrading from rc.6 to rc.7 and adding:

const client = new ApolloClient({
  ...
  fragmentMatcher: new IntrospectionFragmentMatcher(),
  ...

Appears to have broken a few of my initial startup queries (which use fragments), which were previously working in rc.6.

rovansteen · 2017-03-27T09:17:08Z

It doesn't seem to work for me either. I've added the IntrospectionFragmentMatcher, but the data I receive in my component doesn't contain the results from the server anymore.

csillag · 2017-03-27T11:30:47Z

Here are my test results:

Behavior with 1.0.0-rc.6:

loading data with apollo.query - ❌ data is undefined
loading data with apollo.watchQuery - ❌ data is undefined
loading data with the react wrapper - ✅ OK
checking the data after mutation directly in apollo's store - ✅ OK
checking data after mutation with apollo.query - ❌ data is still undefined
reacting to mutations with apollo.watchQuery - ❌ no reaction
reacting to mutations with the react wrapper - ❌ no reaction

Behavior with 1.0.0-rc.7, with `fragmentMatcher` configured:

loading data with apollo.query - ✅ Works
loading data with apollo.watchQuery - ✅ Works
loading data with the react wrapper - ❌ Fails
checking the data after mutation directly in apollo's store - ✅ OK
checking data after mutation with apollo.query - ✅ Correct
reacting to mutations with apollo.watchQuery - ✅ Yes!
reacting to mutations with the react wrapper - ❌ since the react wrapper is defunct, N/A

csillag · 2017-03-27T11:33:54Z

... so, to summarize: rc.7 it works great, except that (for whatever reason) it totally breaks the React integration, which mostly worked, up to this point, even without fragmentMatcher.

csillag · 2017-03-27T12:56:37Z

Further analysis shows that the problem with the React wrapper is caused by the ObservableQuery.currentResult function being broken.

csillag · 2017-03-27T13:40:01Z

To use the introspection fragment matcher for now, you'll also need to specifically provide it:
[...]
I'm doing this because I'd like to make sure that there will not be any unintended consequences of having to fetch (or pre-populate) some schema information before enabling it for everyone by default.

One more bit of info: as far as I can tell, not engaging the the fragmentMatcher doesn't prevent the regression on ObservableQuery.currentResult.

helfer · 2017-03-27T22:13:46Z

Thanks @csillag! That's very interesting, let me look into it.

helfer · 2017-03-27T22:45:13Z

@rovansteen @booboothefool @csillag Could you guys try if your queries work when you pass { options: { notifyOnNetworkStatusChange: true } } to them? That seems to fix the errors I'm seeing in the react-apollo tests.

PS: We recently decided to set the default of notifyOnNetworkStatusChange back to false because it was causing lots of re-renders for people and it was hard for them to track down the issue.

csillag · 2017-03-27T23:56:42Z

Could you guys try if your queries work when you pass { options: { notifyOnNetworkStatusChange: true } } to them?

For me, it doesn't seem to do anything. (currentResult() still returns an empty object for data.)

However, I can confirm that using lastResult instead of currentResult() seems to return the desired result. (Regardless of notifyOnNetworkStatusChange.)

helfer · 2017-03-28T00:01:37Z

Thanks for the feedback @csillag. Would you be able to make a PR with a failing test or provide a reproduction with react-apollo-error-template?

For me all the tests pass with currentResult. Using getLastResult breaks a whole bunch of tests, so I think currentResult still does something we need...

csillag · 2017-03-28T00:03:23Z

I'll try to do that tomorrow.

FWIW, I didn't use getLastResult(); I used lastResult.

helfer · 2017-03-28T00:30:44Z

I think observableQuery.getLastResult() just returns observableQuery.lastResult ;)

csillag · 2017-03-28T15:16:45Z

OK, so I have prepared a reproduction based on react-apollo-error-template.

Please test the following two branches:

csillag/use-fragments - This one uses rc6. The react wrapper works, but running queries normally does.
csillag/use-fragments-rc7 - This one uses rc7 and fragmentMatcher. React wrapper doesn't work, but running queries manually does.

Testing manual query execution

Run this command on the console:

apollo.query({query: getPeople}).then(result => console.log(result.data))

On rc.6, this yields: Loaded data: undefined
On rc.7, this yields: Loaded data: Object {people: Array(3), Symbol(id): "ROOT_QUERY"}

Testing the React wrapper

The loaded data is automatically displayed on both the browser console and rendered on the UI.

rovansteen · 2017-03-30T21:43:30Z

Any progress on this? I see 1.0 is released but it looks like this bug is still there.

csillag · 2017-03-30T21:45:34Z

Rc.9 works for me flawlessly! 2017. márc. 30. 23:43 ezt írta ("Robert van Steen" <notifications@github.com

…

): Any progress on this? I see 1.0 is released but it looks like this bug is still there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1483 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB_y4A5P3RC88seOSRedrdFjYHI4liV5ks5rrCIFgaJpZM4MncH1> .

helfer · 2017-03-31T01:11:35Z

@rovansteen We didn't make it the default, so you have to specifically pass the fragment matcher and initialize it with the relevant part of your schema:

const fm = new IntrospectionFragmentMatcher({
 introspectionQueryResultData: {
  __schema: {
    types: [{
      kind: 'UNION',
      name: 'Item',
      possibleTypes: [{ name: 'GreenItem' }, { name: 'RedItem }]
    }]
  }
});

First we also wanted the fragment matcher initialize itself, but the code ended up being more complicated to maintain than we wanted, so we ditched that for now.

We'll try to provide some instructions for how to seed the fragment matcher with a build script or server-side rendering.

rovansteen · 2017-03-31T08:13:46Z

@helfer Is there any clear documentation on this? I can't get it to work. If I don't setup the IntrospectionFragmentMatcher my first result works fine, but if I use the fetchMore method I get no data, even though the server returns the data without any problems. If I do use the fragmentMatcher, I get no data at all for all my queries. I tried it with a partial schema (just the union part) and a complete schema of everything. Both break all my queries.

Edit: Ok, small correction. The fragmentMatcher breaking all my queries was to a mistake on my side in my schema. Now I've got the schema correct, but the issue remains that fetchMoreResult is undefined on my updateQuery method.

I've created a separate issue with example repo (the one from csillag with a few changes, thanks!) to illustrate this issue. #1523

stayclean · 2017-11-16T07:30:07Z

The problem is solved with the solution, but before we found the solution,
this document which does not work is kind of misleading. Please fix or check the document on the doc website.

This was referenced Mar 24, 2017

Nested fragment results arent't merged into a query result #1479

Closed

Interface query with conditional fragments breaks after v0.8.7 #1363

Closed

bgentry and others added 9 commits March 25, 2017 00:48

failing test: interface query w/ conditional fragments

b547060

This is a failing test to demonstrate issue #1363.

Make fragment on union test fail, not time out

6a0e00a

WIP: Pass fragmentMatcher from client

52c1ca7

Implement first version of IntrospectionFragmentMatcher

d92dff0

Use precompiled introspection query in fragment matcher

a999c10

Carefully thread fragment matcher through to readFromStore

bf5c5de

Make sure fetchQuery waits for FragmentMatcher.init

47db864

Export IntrospectionFragmentMatcher

e700b90

Add explicit types

53948ba

helfer force-pushed the fragment-matcher branch from d72d77e to 53948ba Compare March 25, 2017 07:48

Fix import of QueryManager

e4345f6

stubailo suggested changes Mar 25, 2017

View reviewed changes

Code cleanup based on review feedback

711a695

helfer added 4 commits March 25, 2017 09:15

Add clarifying comment

7c2e729

Update CHANGELOG.md

d7a7561

Make sure introspection fetcher uses cache

65bbe13

Make sure fragment matcher tests do not use addTypename

a811f19

helfer merged commit 581ef1e into master Mar 25, 2017

helfer deleted the fragment-matcher branch March 25, 2017 17:28

helfer changed the title ~~WIP: Fragment matcher~~ Fragment matcher Mar 25, 2017

helfer mentioned this pull request Apr 20, 2017

Query fails with fragments on interfaces, works without #1608

Closed

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

Fragment matcher #1483

Fragment matcher #1483

Conversation

helfer commented Mar 23, 2017 • edited

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

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

helfer commented Mar 25, 2017

stubailo commented Mar 25, 2017

csillag commented Mar 25, 2017

helfer commented Mar 25, 2017

csillag commented Mar 25, 2017

helfer commented Mar 25, 2017

bgentry commented Mar 25, 2017

stubailo commented Mar 25, 2017

helfer commented Mar 26, 2017

helfer commented Mar 26, 2017

stubailo commented Mar 26, 2017

TSMMark commented Mar 27, 2017 • edited

bgentry commented Mar 27, 2017

stubailo commented Mar 27, 2017

booboothefool commented Mar 27, 2017 • edited

rovansteen commented Mar 27, 2017

csillag commented Mar 27, 2017

Behavior with 1.0.0-rc.6:

Behavior with 1.0.0-rc.7, with fragmentMatcher configured:

csillag commented Mar 27, 2017

csillag commented Mar 27, 2017

csillag commented Mar 27, 2017

helfer commented Mar 27, 2017

helfer commented Mar 27, 2017

csillag commented Mar 27, 2017 • edited

helfer commented Mar 28, 2017

csillag commented Mar 28, 2017

helfer commented Mar 28, 2017

csillag commented Mar 28, 2017

Testing manual query execution

Testing the React wrapper

rovansteen commented Mar 30, 2017

csillag commented Mar 30, 2017 via email

helfer commented Mar 31, 2017

rovansteen commented Mar 31, 2017 • edited

stayclean commented Nov 16, 2017

helfer commented Mar 23, 2017 •

edited

TSMMark commented Mar 27, 2017 •

edited

booboothefool commented Mar 27, 2017 •

edited

Behavior with 1.0.0-rc.7, with `fragmentMatcher` configured:

csillag commented Mar 27, 2017 •

edited

rovansteen commented Mar 31, 2017 •

edited