Support notifying subscribers despite the reference not changing

[mfbx9da4]

I'm looking into making an adapter from watermelon DB observables to Legend. In watermelon, when the record changes, the reference never changes. In legend, if the reference doesn't change, the subscribers won't be notified when setting the value. Is there a way to work around this?

legend-state/src/ObservableObject.ts

Line 571 in d183ab6

updateNodesAndNotify(node, newValue, prevValue, childNode, isPrim, isRoot, level);

Perhaps a second parameter such as observable.set(value, { forceNotify: true })

[jmeistrich]

Hmm, that's an interesting one.

The downside of a forceNotify is that it would need to notify at every single in a hierarchy. We currently have optimizations to skip processing children if the child value is unchanged from the previous child value, so setting a huge object would only need to process the nodes that did not change. But with forceNotify it would have to process every single node.

Of course even though it might be slow it's better than nothing... But I wonder if there's a better way to do it? Can you share some of your adapter code so I can see what it looks like and maybe we can find a more optimized solution?

[mfbx9da4]

Sure, here's a draft. Right now, this is untested code. Hopefully it gives you enough context to problem solve though. Let me know if you need a working snippet.

import { useObservable } from '@legendapp/state/react'
import { Model, Query } from '@nozbe/watermelondb'
import { useLayoutEffect } from 'react'

export const WatermelonLegendStateAdapter = {
  useModel: <T extends Model>(model: T) => {
    const o = useObservable(model)
    useLayoutEffect(() => {
      // A value is emitted synchronously when the model is first subscribed to
      return model.experimentalSubscribe(isDeleted => {
        if (isDeleted) {
          // TODO: handle deletion
        } else {
          // HACK: Since the model reference will never change we use a proxy to signal changes
          o.set(new Proxy(model, {}))
        }
      })
    }, [model, o])
    return o
  },
  useQuery: <T extends Query<U>, U extends Model>(query: T) => {
    const o = useObservable([] as U[])
    useLayoutEffect(() => {
      // A value is emitted synchronously when the query is first subscribed to
      return query.experimentalSubscribe(records => o.set(records))
    }, [query, o])
    return o
  },
}

A few things to note:

• It would be more useful and more performant if the observable was a singleton property on the model. Performance wise, I wonder if it would be worth unlinking the Legend Observable when it has no more subscribers?
• Same for query
• It would also be good to support useRelation
• Watermelon supports RxJS so it might be nice to have an adapter between RxJS and Legend to support that API too

Also, just thought it might be worth adding that this could be quite a big opportunity for Legend state and for Watermelon. Watermelon is stuck in the legacy age of HOCs. They've wanted to move to hooks since 2018 but haven't managed yet, partly due to performance. Needless to say, if Legend could provide a stable and performant hooks API for Watermelon, a ton of users would start using Legend.

[jmeistrich]

Ok, I have an idea:

1. Save the previous value from the query. You may need to clone if WatermelonDB is updating the object in place.
2. On change, set the raw value of the observable to the previous value (without notifying)
3. Set the new value, which will then notify anywhere it changed

Setting the raw value at the root of an observable is currently only possible in an experimental new feature: https://legendapp.com/open-source/state/experiments/#enabledirectpeek. But alternatively you could just create it with a child, like const o = useObservable({ value: [] as U[] }). Then you can set the raw value with o.peek().value = ...

const o = useObservable({ value: [] as U[] })
let previousValue

experimentalSubscribe(records => {
    if (previousValue) {
        const data = o.peek() // Get the raw object
        data.value = previousRecords // Set the raw object to the previous value
    }

    o.value.set(records) // Set the new value onto the observable
    
    previousValue = clone(records) // Save the previous value for next time
})

The clone may not be great for performance if these are huge objects, but it would probably be faster than iterating every node of the object and notifying all listeners...

Does that approach make sense?

[mfbx9da4]

Pretty sure we don't need to worry about cloning arrays as the query will emit a new array instance each time based on this line and this line. So unless I'm missing something the useQuery suggestion I posted should just work?

Side note on observable queries in Watermelon, query results only "re-render" if an item is created or deleted, so we don't need to worry about the references of the individual items staying constant. I believe this is similar to how Legend deals with lists anyway?

What do you think about the Proxy hack, it will be proxy on proxy on proxy, could this have perf consequences?

[mfbx9da4]

Do you think it's worth changing the issue title to Feature: Watermelon Adapter or do you think leave as forceNotify?

The downside of a forceNotify is that it would need to notify at every single in a hierarchy.

Even if all the subchildren references have not changed?

[jmeistrich]

Pretty sure we don't need to worry about cloning arrays as the query will emit a new array instance each time based on this line and this line. So unless I'm missing something the useQuery suggestion I posted should just work?

Side note on observable queries in Watermelon, query results only "re-render" if an item is created or deleted, so we don't need to worry about the references of the individual items staying constant. I believe this is similar to how Legend deals with lists anyway?

Oh, it doesn't update if something is changed? In that case it seems like it should be fine.

What do you think about the Proxy hack, it will be proxy on proxy on proxy, could this have perf consequences?

I think that wouldn't work because although it sees the outer object as difference the references inside the object would still be the same. But I do think the idea of setting previous should work there, but with the model instead of the records array? I'm not super familiar with WatermelonDB, and from some research it seems like you're using undocumented experimental subscribers? So I'm guessing how it should work, but I think that should be right?

[jmeistrich]

Do you think it's worth changing the issue title to Feature: Watermelon Adapter or do you think leave as forceNotify?
Let's keep talking to figure out what the actual thing to be done is, then change it :)

The downside of a forceNotify is that it would need to notify at every single in a hierarchy.

Even if all the subchildren references have not changed?

I think a forceNotify would have to force notifying at every single node in the object? Or maybe I'm misunderstanding what you're wanting it to do. From your description I'd thought that WatermelonDB is updating the model in place so setting has no effect because it's not changed. But it does that check at every node in the object so it would need to go through all child nodes recursively and notify them. If it only notified for the root and not the children it would break the core behavior of notifying sub-nodes when they change.

[mfbx9da4]

I'm currently getting a recursive call to updateNodes with the following code

const o = observable([] as any[])
const q = database.collections.get<Post>('posts').query()
q.experimentalSubscribe(x => {
  o.set(x) // Causes RangeError: Maximum call stack size exceeded the first time this is called
})

I think ultimately this is because each watermelon record object has a circular data structure, how can I work around this?

[mfbx9da4]

Ah I found opaqueObject seems to be working now?

[mfbx9da4]

Hmm, when trying to update the object with the following standard watermelon code

          await database.write(async () => {
            await result.update(x => {
              x.title = `Updated ${counter.current++}`
            })
          })

I get the error

TypeError: Proxy set returned false for property '_isEditing'

Likely this is because of my use of opaqueObject. Here is my latest code:

export const WatermelonLegendStateAdapter = {
  useRecord: <T extends Model>(record: T) => {
    const o = useObservable(record)
    useLayoutEffect(() => {
      // A value is emitted synchronously when the model is first subscribed to
      return record.experimentalSubscribe(isDeleted => {
        if (isDeleted) {
          // TODO: handle deletion
        } else {
          o.set(opaqueObject(record))
        }
      })
    }, [record, o])
    return o
  },
  useQuery: <T extends Model>(query: Query<T>) => {
    const o = useObservable([] as T[])
    useLayoutEffect(() => {
      // A value is emitted synchronously when the query is first subscribed to
      return query.experimentalSubscribe(records => {
        o.set(records.map(x => opaqueObject(x)))
      })
    }, [query, o])
    return o
  },
}

I imagine this is because opaqueObject blocks mutation? But then I also need opaqueObject to avoid circular issues. How can I get around this?

[jmeistrich]

opaqueObject doesn't specifically block mutations, but direct modifications on an observable are blocked (because that's usually an error). Where is _isEditing being set from? It may be that you're saving the observable object into WatermelonDB which is internally changing _isEditing?

Also, opaqueObject would solve the circular references problem because it stops it from iterating through nodes looking for listeners to update, but that also means you wouldn't be able to listen to any child properties.

[mfbx9da4]

isEditing is set internally by watermelon when an update closure is run, I’ll send you the line later

[mfbx9da4]

https://github.com/Nozbe/WatermelonDB/blob/7ad34099b863054da26bf705b64c8cba3a840990/src/Model/index.js#L127 there you are

[mfbx9da4]

You were right, it was a direct modification of an observable. I did a hack to make the circular reference not enumerable which seems to work, this is where I'm at

import { isObservable } from '@legendapp/state'
import { useObservable, useObserveEffect } from '@legendapp/state/react'
import { Model, Query } from '@nozbe/watermelondb'
import { useLayoutEffect } from 'react'

export const WatermelonLegendStateAdapter = {
  useRecord: <T extends Model>(record: T) => {
    const o = useObservable<T>(isObservable(record) ? record.peek() : record)
    useLayoutEffect(() => {
      // A value is emitted synchronously when the model is first subscribed to
      return record.experimentalSubscribe(isDeleted => {
        console.log('>>> record title', (record as any).peek().title)
        if (isDeleted) {
          // TODO: handle deletion
        } else {
          // HACK: create a new reference to force update
          o.set(new Proxy(isObservable(record) ? record.peek() : record, {}))
        }
      })
    }, [record, o])
    return o
  },
  useQuery: <T extends Model>(query: Query<T>) => {
    const o = useObservable([] as T[])
    useLayoutEffect(() => {
      // A value is emitted synchronously when the query is first subscribed to
      return query.experimentalSubscribe(records => {
        o.set(records.map(x => x))
      })
    }, [query, o])
    return o
  },
}

It does seem to be reactive and working for the limited use cases that I've tried

[mfbx9da4]

Hey so the general approach above seems to be working nicely.

I just have one question, Legend does seem to be significantly slower than Watermelon's homegrown withObservables solution. The extreme case I have profiled is with useQuery with 6000 items returned rerendering a single component. Legend ~12ms vs watermelons ~4ms. It seems that Legend traverses every property of every item in the array inside the function updateNodes. Why is this? The reference for almost every single item is the same so it should opt out of this? My understanding is that a node is a proxy over the underlying tree data structure used for tracking listeners? But why would there be a listener on every item since I'm just doing useQuery?

[jmeistrich]

Are you sure the reference is the same? It should skip over any elements that are the same:

legend-state/src/ObservableObject.ts

Line 216 in 86d66f3

if (isDiff) {

Is this on the initial load where it's adding all new data to the observable or is it an update that's changing them?

[mfbx9da4]

Thanks for the quick response, I'm fairly sure it's not from the initial response. I'll verify this when back at a keyboard later this week.

[mfbx9da4]

I dug a bit deeper. You were right, the recursive updateNodes cost was on the initial load not subsequent updates. Though it's not clear to me why there would need to be a recursive calls to updateNodes when creating the observable, couldn't all the nodes be created lazily? I couldn't track down the root of the performance difference between watermelon and legend on updates.

However, I did realize I was on an old version of Legend (0.21.18). I upgraded to latest (1.11.1) which caused all my tests to fail because my proxy hack above, didn't result in the observable updating. I changed the following line

legend-state/src/ObservableObject.ts

Line 705 in 86d66f3

hasADiff = updateNodes(childNode, newValue, prevValue);

to

hasADiff = updateNodes(childNode, newValue, prevValue) || prevValue !== newValue;

and all my tests passed again. Also, now that I've upgraded to v1 the performance is much better. The performance degradation between Legend and Watermelon is now negligible. Which is great news 🥳! So I guess all the nodes are being created lazily in v1? I'll have to do some more rigorous benchmarking to verify there is a negligible cost to Legend.

Without the above change / using patch-package, Legend wouldn't work for my use case. What do you think of my change? Do you think it's worth adding forceNotify parameter to set?

[jmeistrich]

Hmm, that's an interesting issue. I can't think of a better way to solve it at the moment, but I'll think about it some more.

That hack would just notify at the root level but not at any of the children, right? So it would work for the initial load but would not update child nodes that they've changed in later updates?

[mfbx9da4]

I think it will work for initial load and root updates.

The actual updates to the underlying record happen outside of legend so when it comes to doing the diffing in updateNodes it's too late! useComputed(() => record.get().column) does seem to work well though. What could break? Probably useComputed(() => record.column.get()) won't work. These records are not deeply nested though so that's probably acceptable for my use case.

Maybe I should revisit opaqueObject?

Maybe I'm just holding Legend wrong ¯_(ツ)_/¯

[mfbx9da4]

Perhaps a better approach would be to wrap the prepareUpdate method on all the models with observable.set? 🤔

https://github.com/Nozbe/WatermelonDB/blob/0974c47a559fa4f102f0026afb62d0ee368d01ca/src/Model/index.js#L124

[chakrihacker]

Hey @mfbx9da4 Why are you wrapping legend state with watermelon db??