proxyWithComputed and subscribeKey

[lukebarlow]

Should we be able to use subscribeKey on a computed value? This test currently fails

describe('proxyWithComputed and subscribeKey', () => {
  it('should call subscribeKey subscription when computed value changes?', async () => {
    const state = proxyWithComputed({
      count: 1,
    }, {
      doubled: snap => snap.count * 2
    })
    const handler = jest.fn()
    subscribeKey(state, 'doubled', handler)
    state.count = 2
    await Promise.resolve()
    expect(handler).toBeCalledTimes(1)
  })
})

[dai-shi]

Good point. Does subscribe work as expected with proxyWithComputed?

[lukebarlow]

Yes, subscribe works and subscribeKey works with the regular properties, just not the computed ones

…

On Wed, 2 Feb 2022, 14:56 Daishi Kato, ***@***.***> wrote: Good point. Does subscribe work as expected with proxyWithComputed? — Reply to this email directly, view it on GitHub <#350 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEP4NB2Y2GPA22ZNU6KTRTUZFA3JANCNFSM5NMIPSPQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[dai-shi]

Ah, now I get it.
It's because how subscribeKey is implemented now. So, it can be said, it works so by design.

We used to have a different implementation, which is changed in #177.
The old one is this:

export const subscribeKey = <T extends object>(
  proxyObject: T,                                               
  key: keyof T,
  callback: (value: T[typeof key]) => void,
  notifyInSync?: boolean        
) => {                                                          
  let prevValue = proxyObject[key]           
  return subscribe(                      
    proxyObject,                
    () => {           
      const nextValue = proxyObject[key]     
      if (!Object.is(prevValue, nextValue)) {
        callback((prevValue = nextValue))
      }     
    },                                                     
    notifyInSync                
  )
}

The old one passes your test.

I wonder if we should revert it, or accept the limitation (the proxyWithComputed and subscribeKey combo doesn't work.)

[DennisSmolek]

It's because how subscribeKey is implemented now. So, it can be said, it works so by design.

We used to have a different implementation, which is changed in #177. The old one is this:

Can you clarify why the new version isn't compatible?

[dai-shi]

The new version of subscribeKey checks what key is changed in the "operation"s, so it doesn't "read" the computed value.
The old version checks if the new value is changed from the previous value using Object.is, so it triggers the computation inside the callback function.

[lukebarlow]

I leave it to you guys to decide which version you want, but I can explain I have a specific use case which I was trying to implement which lead to finding this issue. Essentially I have two calculated properties, one which triggers heavy updating of the UI when it changes and another which triggers a lighter update. They depend on different properties of the object, so update at different times. In order to make the UI responsive, I need to be able to detect the changes to the two calculated properties independently.

I have also tried with derived, but came across #349 .

I'm sure I can figure out a workaround, but proxyWithComputed seems like it would be the most elegant and readable solution, so I think it would be cool if it worked.

[dai-shi]

Implementation-wise, I would like to keep the newer version, because the older version can easily be implemented by a user.

So, in your use case, what is the real problem with proxyWithComputed and subscribe combo?

If that really needs it, can you define this on your end?

const subscribeKey = <T extends object, K extends keyof T>(
  proxyObject: T,
  key: K,
  callback: (value: T[K]) => void
) => {
  let prevValue = proxyObject[key]
  return subscribe(
    proxyObject,
    () => {
      const nextValue = proxyObject[key]
      if (prevValue !== nextValue) {
        callback(prevValue = nextValue)
      }
    }
  )
}

[lukebarlow]

Okay, that will work. Thanks

[dai-shi]

I will leave the current implementation just because it's harder for library users to implement (which uses ops).

I think we should cover this limitation and workaround in docs eventually, but closing this for now.

[sarimarton]

This is definitely a bug, given that subscribeKey is typed on the second parameter, which offers the calculated fields, and it's a lie. Either the typing should be updated (at least dumbed down to string, if not possible to separate the calculated/non-calculated fields on the type level), or the older, more capable version should be kept in the library.

IMO, from the output of the proxy creation, whether a key is calculated or not should be irrelevant at the consumption side. If I had a dilemma of which one of competing implementations to keep in the lib as default, I'd definitely set my preference along the feature/capability aspect, rather than whether how hard users could potentially re-implement them. Users hardly care about that. On the other side, I, as a developer in a team who's responsible for recommending coding practices, have a much harder case to justify this state library when they see that I have to locally patch the lib.

I understand the technical concern. Isn't there a way to include the old one as well in the lib? I would leave the old one, and add the new one under an experimental import namespace with a dumbed down typing on the key.

[dai-shi]

Totally valid comment. Thanks for raising it again. Hm, yeah, I think we can revert to the original one, as new one doesn't provide any capabilities.