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 ***@***.***> 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 <#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 ***@***.***> 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 ***@***.***> 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 > <#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 ***@***.***> 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 ***@***.***> 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 > <#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 <#412 (comment)>, 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 ***@***.***> 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 ***@***.***> 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 ***@***.***> 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 >> <#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 <#412 (comment)>, 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