writeFragment blows away entity's __typename

[finnigantime]

Summary:

Say existingFoo is an entity in the cache with this data:

{
  __typename: 'Foo',
  id: '123',
  bar: oldBarValue,
  baz: 'bazValue'
}

We call writeFragment() to update it's .bar:

    proxy.writeFragment({
      id: existingFoo.id,
      fragment: fragments.fooWithBar,
      fragmentName: 'fooWithBar',
      data: {
        id: existingFoo.id,
        bar: newBarValue,
      },
    });

Expected Behavior:

{
  __typename: 'Foo',
  id: '123',
  bar: newBarValue,
  baz: 'bazValue'
}

Actual Behavior:

{
  __typename: undefined,
  id: '123',
  bar: newBarValue,
  baz: 'bazValue'
}

This happens when addTypename is true in the config since the fragment we are writing gets transformed and __typename gets added to it. If we don't similarly include __typename: 'Foo' in the data arg of writeFragment, the existingFoo.__typename gets deleted.

This is not obvious and at the least should result in a warning. Ideally it shouldn't happen at all (when writing a subfragment, I don't think __typename should be deleted if it wasn't specified on the subfragment). Recipes for writeFragment don't indicate the need to add __typename (https://www.apollographql.com/docs/angular/basics/caching#writequery-and-writefragment) and it has caused problems for users (https://stackoverflow.com/questions/48631954/apollo-writefragment-not-updating-data).

[nevir]

If the id has changed from what previously existed at that position in the graph, this is expected (you're replacing the entity with a new one, and Hermes can't make assumptions about fields from the previous entity - what if it's a union type?)

On Fri, Apr 5, 2019, 12:44 Pat Finnigan [email protected] wrote:

Summary:

Say existingFoo is an entity in the cache with this data:

{ __typename: 'Foo', id: '123', bar: oldBarValue, baz: 'bazValue' }

We call writeFragment() to update it's .bar:

proxy.writeFragment({
  id: existingFoo.id,
  fragment: fragments.fooWithBar,
  fragmentName: 'fooWithBar',
  data: {
    id: existingFoo.id,
    bar: newBarValue,
  },
});

Expected Behavior:

{ __typename: 'Foo', id: '123', bar: newBarValue, baz: 'bazValue' }

Actual Behavior:

{ __typename: undefined, id: '123', bar: newBarValue, baz: 'bazValue' }

This happens when addTypename is true in the config since the fragment we are writing gets transformed and __typename gets added to it. If we don't similarly include __typename: 'Foo' in the data arg of writeFragment, the existingFoo.__typename gets deleted.

This is not obvious and at the least should result in a warning. Ideally it shouldn't happen at all (when writing a subfragment, I don't think __typename should be deleted if it wasn't specified on the subfragment). Recipes for writeFragment don't indicate the need to add __typename ( https://www.apollographql.com/docs/angular/basics/caching#writequery-and-writefragment) and it has caused problems for users ( https://stackoverflow.com/questions/48631954/apollo-writefragment-not-updating-data ).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412, or mute the thread https://github.com/notifications/unsubscribe-auth/AAChnZBRvoDNcEcHJG5AHWIEzzTwYDpOks5vd6eEgaJpZM4cfmrh .

[nevir]

General guidance is that you'll need to provide __typename when calling writeFragment (or any direct write to the cache) to ensure that it stays consistent

On Fri, Apr 5, 2019, 12:47 Ian MacLeod [email protected] wrote:

If the id has changed from what previously existed at that position in the graph, this is expected (you're replacing the entity with a new one, and Hermes can't make assumptions about fields from the previous entity - what if it's a union type?)

On Fri, Apr 5, 2019, 12:44 Pat Finnigan [email protected] wrote:

Summary:

Say existingFoo is an entity in the cache with this data:

{ __typename: 'Foo', id: '123', bar: oldBarValue, baz: 'bazValue' }

We call writeFragment() to update it's .bar:

proxy.writeFragment({
  id: existingFoo.id,
  fragment: fragments.fooWithBar,
  fragmentName: 'fooWithBar',
  data: {
    id: existingFoo.id,
    bar: newBarValue,
  },
});

Expected Behavior:

{ __typename: 'Foo', id: '123', bar: newBarValue, baz: 'bazValue' }

Actual Behavior:

{ __typename: undefined, id: '123', bar: newBarValue, baz: 'bazValue' }

This happens when addTypename is true in the config since the fragment we are writing gets transformed and __typename gets added to it. If we don't similarly include __typename: 'Foo' in the data arg of writeFragment, the existingFoo.__typename gets deleted.

This is not obvious and at the least should result in a warning. Ideally it shouldn't happen at all (when writing a subfragment, I don't think __typename should be deleted if it wasn't specified on the subfragment). Recipes for writeFragment don't indicate the need to add __typename ( https://www.apollographql.com/docs/angular/basics/caching#writequery-and-writefragment) and it has caused problems for users ( https://stackoverflow.com/questions/48631954/apollo-writefragment-not-updating-data ).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412, or mute the thread https://github.com/notifications/unsubscribe-auth/AAChnZBRvoDNcEcHJG5AHWIEzzTwYDpOks5vd6eEgaJpZM4cfmrh .

[nevir]

Ah, the alternative is that it is a subtle bug. Because we have addTypename enabled, we are implicitly asking for __typename in the fragment passed to the cache, and because it's not present in the data, it dutifully removes it

On Fri, Apr 5, 2019, 12:47 Ian MacLeod [email protected] wrote:

If the id has changed from what previously existed at that position in the graph, this is expected (you're replacing the entity with a new one, and Hermes can't make assumptions about fields from the previous entity - what if it's a union type?)

On Fri, Apr 5, 2019, 12:44 Pat Finnigan [email protected] wrote:

Summary:

Say existingFoo is an entity in the cache with this data:

{ __typename: 'Foo', id: '123', bar: oldBarValue, baz: 'bazValue' }

We call writeFragment() to update it's .bar:

proxy.writeFragment({ id: existingFoo.id, fragment: fragments.fooWithBar, fragmentName: 'fooWithBar', data: { id: existingFoo.id, bar: newBarValue, }, });

Expected Behavior:

{ __typename: 'Foo', id: '123', bar: newBarValue, baz: 'bazValue' }

Actual Behavior:

{ __typename: undefined, id: '123', bar: newBarValue, baz: 'bazValue' }

This happens when addTypename is true in the config since the fragment we are writing gets transformed and __typename gets added to it. If we don't similarly include __typename: 'Foo' in the data arg of writeFragment, the existingFoo.__typename gets deleted.

This is not obvious and at the least should result in a warning. Ideally it shouldn't happen at all (when writing a subfragment, I don't think __typename should be deleted if it wasn't specified on the subfragment). Recipes for writeFragment don't indicate the need to add __typename (

https://www.apollographql.com/docs/angular/basics/caching#writequery-and-writefragment )

and it has caused problems for users (

https://stackoverflow.com/questions/48631954/apollo-writefragment-not-updating-data

).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAChnZBRvoDNcEcHJG5AHWIEzzTwYDpOks5vd6eEgaJpZM4cfmrh

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412#issuecomment-480400559, or mute the thread https://github.com/notifications/unsubscribe-auth/AAChnZYnOmJ5Ftf9CW0zTemQgxxS41D-ks5vd6hmgaJpZM4cfmrh .

[nevir]

I don't think we should explicitly ignore _typename due to the union case, though... Hmm

On Fri, Apr 5, 2019, 12:48 Ian MacLeod [email protected] wrote:

General guidance is that you'll need to provide __typename when calling writeFragment (or any direct write to the cache) to ensure that it stays consistent

On Fri, Apr 5, 2019, 12:47 Ian MacLeod [email protected] wrote:

If the id has changed from what previously existed at that position in the graph, this is expected (you're replacing the entity with a new one, and Hermes can't make assumptions about fields from the previous entity - what if it's a union type?)

On Fri, Apr 5, 2019, 12:44 Pat Finnigan [email protected] wrote:

Summary:

Say existingFoo is an entity in the cache with this data:

{ __typename: 'Foo', id: '123', bar: oldBarValue, baz: 'bazValue' }

We call writeFragment() to update it's .bar:

proxy.writeFragment({ id: existingFoo.id, fragment: fragments.fooWithBar, fragmentName: 'fooWithBar', data: { id: existingFoo.id, bar: newBarValue, }, });

Expected Behavior:

{ __typename: 'Foo', id: '123', bar: newBarValue, baz: 'bazValue' }

Actual Behavior:

{ __typename: undefined, id: '123', bar: newBarValue, baz: 'bazValue' }

This happens when addTypename is true in the config since the fragment we are writing gets transformed and __typename gets added to it. If we don't similarly include __typename: 'Foo' in the data arg of writeFragment, the existingFoo.__typename gets deleted.

This is not obvious and at the least should result in a warning. Ideally it shouldn't happen at all (when writing a subfragment, I don't think __typename should be deleted if it wasn't specified on the subfragment). Recipes for writeFragment don't indicate the need to add __typename (

https://www.apollographql.com/docs/angular/basics/caching#writequery-and-writefragment )

and it has caused problems for users (

https://stackoverflow.com/questions/48631954/apollo-writefragment-not-updating-data

).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAChnZBRvoDNcEcHJG5AHWIEzzTwYDpOks5vd6eEgaJpZM4cfmrh

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/convoyinc/apollo-cache-hermes/issues/412#issuecomment-480400841, or mute the thread https://github.com/notifications/unsubscribe-auth/AAChnZVOw_6cdazfZnThMFFMQEMC2hN4ks5vd6imgaJpZM4cfmrh .

[finnigantime]

@nevir Yeah I think it's the latter. In this case the ID has not changed, we're persisting a small update to the existing entity.

[nevir]

@nevir Yeah I think it's the latter. In this case the ID has not changed, we're persisting a small update to the existing entity.

Ah doh yeah, misread your initial example 👍

Recommendation for update cases like that would be to spread the previous version of the object into your data

[nevir]

I think really what you want here is for Hermes to support unvalidated writes (e.g. writeData)

[nevir]

Another potential option could be for hermes to assert that __typename is present in all nodes when addTypename is true

[finnigantime]

@nevir yeah I think writeData may be ideal. Spreading the existing object value makes sense and works, but this still seems like a gotcha since the cache supports partial fragment merging and the writeFragment examples don't suggest including __typename or ...existingFoo. I like the assertion idea. I was thinking of adding a warning around this.

[nevir]

FWIW, I'm pretty sure inmemory cache has the same behavior here