Added tests for subscribe

[wilcoschoneveld]

Hey! Thanks for the work on this library, I would like to contribute.

I have added tests for the subscribe function, and would also like to fix the non-working ones:

1. When you call subscribe on a non-object property (count in this case) it throws an Cannot read property 'add' of undefined error. I'm not quite sure how to go around fixing this, my guess is to do something like a createDeepProxy much earlier?

2. When you change the state inside a subscription, it calls the subscription again leading to a Maximum call stack size exceeded exception. I think this isDeepChanged needs to be moved to into proxy in vanilla?

Any pointers in the right direction? Thanks a lot!

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dacad4e:

Sandbox	Source
React	Configuration
React Typescript	Configuration

[dai-shi]

Thanks so much for your interest and contribution.

Here're some random/incomplete notes:

• proxy-compare is for read operation and only used in useProxy
• for mutating state (vanilla.ts), we don't need proxy-compare (there's one exception in the code)
• subscribe only accepts a proxy, so a number (state.count) doesn't work
• so, the issue is how to show better error messages
• we have some discussions about error messages ( cc @a-type )
• infinite loop caused by subscribe(p, () => { p.count = 0 }) seems fixable by comparing with previous value
• however subscribe(p, () => { p.obj = { a: 1 } }) can't be detected so we may try to detect subscribe/notify loop. (requires deeper thoughts)

[wilcoschoneveld]

Thanks for your reply!

I'm struggling to get my head around the exact workings of this lib, I'm not really familiar with proxies yet.

subscribe only accepts a proxy, so a number (state.count) doesn't work

In the following example:
https://codesandbox.io/s/pensive-smoke-i4ze6?file=/src/App.js

how exactly does useProxy know that it should render on A and not re-render on B?

Maybe we can use a similar technique to allow subscribing to state.count, and also prevent re-run of handlers.

Thanks for your time!

[dai-shi]

I can understand how you struggle with these.
One is due to lack of internal docs, which is my fault and something I'm not super good at.
Hope some discussions here clarify things and lead to some docs...
This lib uses proxies but we have a dependency lib proxy-compare which also uses proxies.
In fact, we use proxies in two ways.

1. proxies for global state and tracking mutation
2. proxies for render optimization in react (proxy-compare)

Between 1 and 2, we have snapshots that are pure objects (no proxies)

They work totally differently. Theoretically, we could use the same technique in 2 to track read operation in 1, which means we use proxy-compare in 1.
I don't recommend for two reasons: a) proxy-compare is very complex and even I don't feel like doing it (btw, I assume mobx does 1 and 2 at the same time), and b) it's quite costly for just avoiding subscribe infinite loop.

I'm not sure how far you read the code, but we use version to track if objects are mutated or not. So, with using it, I think we can avoid invoking subscribe callback while invoking the callback. Or even we remove the callback temporarily.

const listeners = new Set()
const notify = () => {
  new Set(listners).forEach((listener) => {
    listeners.delete(listener)
    listener()
    listners.add(listener)
  })
}

Does this work? If it does, it would be much simpler.
(Ah, we can't avoid mutual recursion with this...)

[dai-shi]

@wilcoschoneveld Let me merge this PR as I'd like to add some tests in subscribe.test.tsx.
I will mark non-working tests skip for now. Would you create a new issue for further discussion?

[wilcoschoneveld]

Yep cool