Improving store write performance by avoiding unnecessary processing …

[abergenw]

…of identical data when writing the store.

As mentioned in #1409 there is some overprocessing occurring when writing to the store IF the same data is returned multiple times for the same graphql field. This PR allows short circuiting when running into an object that has already been processed.

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

[helfer]

@abergenw thanks a lot for this great PR! I definitely want to see this get merged, but it needs to test a bunch of corner cases before I'm confident enough to do so.

Does this use === to compare fields? What would happen in a query where an object is queried with one selection set in one place, and a different selection set the other place? For example

query {
  users {
    id
    name
    address {
      city
     }
  }
  friends {
    id
    name
    address {
      city
      street
    }
  }
}

specifically, would address be the same in both cases and thus not get written with the extra street field the second time?

Same question about aliases.

Would be great to see tests for those (and other) cases.

[abergenw]

@helfer the whole point is avoiding writing the same object multiple times if it's occurring multiple times in the same graphql path (referential equality of field). There would be no short circuiting in the query you provided as the graphql paths are different. However, the following query/response would get short circuited:

    const query = gql`
      {
        id,
        array1 {
          id
          stringField
          obj {
            id
            stringField
            numberField
          }
        }
      }
    `;

    const result: any = {
      id: 'a',
      array1: [
        {
          id: 'aa',
          stringField: 'string',
          obj: {
            id: 'aaa',
            stringField: 'string',
            numberField: 1,
          },
        },
        {
          id: 'ab',
          stringField: 'string2',
          obj: {
            id: 'aaa',
            stringField: 'should not be written',
            numberField: 2,
          },
        },
      ],
    };

I added a few test cases (and fixed a bug =)) but none covering aliased fields. Is this necessary, there is no special case for aliases in writeQueryToStore?

[stubailo]

stringField: 'should not be written',

Should we try to detect when this is inconsistent and maybe throw an error?

[abergenw]

Not sure what you mean @stubailo. We deliberately want writeQueryToStore to skip processing/writing that second occurrence of object with id aaa as this should always be identical to the one already written/processed.

[stubailo]

as this should always be identical to the one already written/processed.

Yep! I'm just wondering if it's helpful to highlight errors in the case when it's not consistent, since that might mean for the developer that they have made a mistake. But that could be part of a different PR.

[abergenw]

If we check for inconsistency I don't think there's much performance improvements to be gained...

[helfer]

The added tests are great, just found a tiny typo. If comparison is ===, then aliased nodes should not be identical, and thus the paths would be different. If that's the case, I'm happy to merge this without including specific tests for that.

[helfer]

"properly"

[abergenw]

The strict equality check === is performed on 'nodes' in the GraphQL AST. If we for the same node found multiple objects with the same id we skip processing of these. It shouldn't matter if the GraphQL AST node is aliased or not.

[chaffeqa]

Should we try to detect when this is inconsistent and maybe throw an error?

You could maybe put this behind a flag of NODE_ENV != 'production

[jbaxleyiii]

@helfer @stubailo @abergenw where did we end on this one? Is it good to merge after fixing conflicts?

[helfer]

@jbaxleyiii last I checked it was pretty close to ready, I just didn't have time to take another close look to make sure it's really good to merge.

[jbaxleyiii]

@abergenw Let's merge this! Thanks again for helping make Apollo faster! 🚀