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

Support inline fragments for diffing + remove Node refetching code #175

Merged
merged 11 commits into from
May 5, 2016

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented May 4, 2016

TODO:

  • Update CHANGELOG.md with your change
  • 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

Fixes #147

@stubailo
Copy link
Contributor Author

stubailo commented May 4, 2016

Note: To properly evaluate fragments, we need __typename. We can probably fake it for now, but it would be good to have #27 to make this feature work much better.

@stubailo stubailo force-pushed the inline-fragments branch from 4fc2fad to 315837a Compare May 4, 2016 23:24
@stubailo stubailo force-pushed the inline-fragments branch from d378c45 to 7ff1a87 Compare May 5, 2016 02:25
@stubailo
Copy link
Contributor Author

stubailo commented May 5, 2016

@jbaxleyiii just wanted to get a sanity check from you - this PR removes a bunch of code around refetching individual nodes that we weren't actually using in the public API at all. If we put it back, it should be as a diffing plugin a la #26

Also, I left some of the old tests in a commented file, should I do that or just delete stuff altogether?

@stubailo stubailo changed the title WIP: Support inline fragments for diffing Support inline fragments for diffing + remove Node refetching code May 5, 2016
@stubailo
Copy link
Contributor Author

stubailo commented May 5, 2016

Deleted the commented code, we can always get it later if we want it.

@jbaxleyiii
Copy link
Contributor

@stubailo what is the goal of removing the node fetching? I thought this was being used because we ran into the need to add support for scalar arrays? (b217b84)

@stubailo
Copy link
Contributor Author

stubailo commented May 5, 2016

The code you're talking about is still there: https://github.com/apollostack/apollo-client/pull/175/files#diff-f3e452947f5135c9e8a6f750e40ccb32R262

What I removed is the ability to print Relay-style node(id: 'abc') {...} queries, since those were only half-implemented.

@stubailo
Copy link
Contributor Author

stubailo commented May 5, 2016

I'd like to merge this soon, let me know if I can clarify better what I'm getting rid of.

@jbaxleyiii
Copy link
Contributor

@stubailo ah! I get it, 🎉 merge through!

@stubailo stubailo merged commit 2625108 into master May 5, 2016
@WilliamHolmes
Copy link

WilliamHolmes commented May 6, 2016

@stubailo
I'm using inline fragments ie.

node(id: 'somePersonId') {... on Person { email } }

is this now supported in 3.0.2? I'm a little confused with a previous comment ...

What I removed is the ability to print Relay-style node(id: 'abc') {...} queries, since those were only half-implemented.

and PR #179

If it's supported, this is excellent news. When do you think 3.0.2 will be published to NPM ? (I guess this is also a Q for the react-apollo module)

@stubailo
Copy link
Contributor Author

stubailo commented May 6, 2016

@WilliamHolmes it should already be published, I think? Try 0.3.2.

I think my comment about Relay nodes is a bit confusing - it's safe to just ignore it, I'm just trying to say that I removed some code that didn't fully work anyway.

Inline fragments should work everywhere now, let me know if they don't.

@jbaxleyiii jbaxleyiii deleted the inline-fragments branch May 17, 2016 02:04
@d-winter
Copy link

d-winter commented Jul 6, 2016

Hi @stubailo,
I have got a problem when using multiple fragments within a query:

Here is my query:

const mapQueriesToProps = ({ ownProps, state }) => {
  return {
    tableDetails: {
      query: gql`
        query table($id: ID!) {
          table(id: $id) {
            _id
            name
            apiID
            desc
            created
            updated
            owner
            fields {
              ... on NumberField {
                _id
                name
                type
                apiID
                desc
                req
              }
              ... on TextField {
                _id
                name
                apiID
                desc
                req
              }
            }
          }
        }
    `,
      variables: {
        id: ownProps.params.id,
      },
      forceFetch: false,
    },
  };
};

Using this query with graphiql works perfectly. With apollo client (v 3.2.6) there occurs the following error:

writeToStore.js:53 Uncaught Error: Can't find field type on result object ROOT_QUERY.table({"id":"577d214ce59e6dd7e7781271"}).fields.1.

The response from apollo server looks good:

{
  "data": {
    "table": {
      "_id": "577d214ce59e6dd7e7781271",
      "name": "Foo",
      "apiID": "bar",
      "desc": "1234",
      "created": "Wed Jul 06 2016 17:18:36 GMT+0200 (CEST)",
      "updated": "Wed Jul 06 2016 17:18:36 GMT+0200 (CEST)",
      "owner": "auth0|5778d3af934728da6af916ee",
      "fields": [
        {
          "_id": "577d373b5d7b8b65eecbf624",
          "name": "test number field",
          "type": "DECIMAL",
          "apiID": "tnf",
          "desc": null,
          "req": true
        },
        {
          "_id": "577d37665d7b8b65eecbf625",
          "name": "test text field",
          "apiID": "ttf",
          "desc": "fooo",
          "req": false
        }
      ]
    }
  }
}

The client seems to use the fields from within the first fragment also within the second?
I am new to graphql, so maybe this is my fault in my implementation?
Thanks a very lot for your help and this incredible project!

@stubailo
Copy link
Contributor Author

stubailo commented Jul 6, 2016

@d-winter hey, thanks for commenting - in the future, please open a new issue since not many people will see a comment on a closed PR.

This is definitely a real bug, and someone recently opened an issue here: #354

We'll try to fix it ASAP, here's an in-progress PR: #356

@d-winter
Copy link

d-winter commented Jul 6, 2016

Hi @stubailo
sorry didn't see that issue. Thanks again!:)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants