Refactor to minimize the number of redundant iterations

[tarabishy2020]

I was going through the code and noticed that SubscribeP function and parseProxyOps are a bit confusing as there are redundant iterations going on there.

parseProxyOps works on the whole set of operations, but those could be related to different paths, yet it's still being called multiple times for each operation.

The refactor makes sure parseProxyOps is called one time. And arrayOps are processed on a per path basis.

[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 3d45aec:

Sandbox	Source
React	Configuration
React Typescript	Configuration

[dai-shi]

Thanks for working on it! Started to slowly review the code. Here's the first question.

[dai-shi]

Isn't there a way to do it without changing the signature?

[tarabishy2020]

I could maybe call it partitionedProxyOps, call that from subscribeP and leave the original as is, or you were thinking of a different option?

[dai-shi]

I'm thinking not changing the exported function at all. I might be missing your refactoring point though.

[dai-shi]

The refactor makes sure parseProxyOps is called one time. And arrayOps are processed on a per path basis.

Maybe my question is whether we can ensure calling the function once, without processing on a per-path basis.

[tarabishy2020]

Got you. Yes, we could do that. But then there is no escaping the redundant runs that happen in the aOps forEach.
So below I am just printing the number of iterations for the below example

import * as Y from 'yjs';
import { proxy } from 'valtio/vanilla';
import { bind } from '../src/old/index';
const initData = [...Array(9).keys()];
test.only('dev', async () => {
  const doc = new Y.Doc();
  const a = doc.getMap('arr');

  const p = proxy<any>({
    x: [...initData],
    y: [...initData],
    z: [...initData],
    i: [...initData],
    j: [...initData],
    k: [...initData],
  });
  bind(p, a);
  (() => {
    p.x.reverse();
    p.y.reverse();
    p.z.reverse();
    p.i.reverse();
    p.j.reverse();
    p.k.reverse();
  })();

  expect(true).toBe(true);
});

Before this refactor

after

[dai-shi]

I would use a WeakMap for snapshot(proxyObj) to avoid the duplication.

I wonder if we can write a failing test firstly.

[dai-shi]

Is the use of pathkey important of the refactoring? Maybe so.
I'm not a big fan of usign . delimiter. What if a path includes the dot?

[tarabishy2020]

I don't think it would be of impact here, since we never break on . again, it's merely just a concatenated representation for the parent path, so we can use it as a key to refer to a collection of arrayOps together.

[dai-shi]

Technically, ["a.b", "c"] and ["a", "b.c"] can conflict.

[dai-shi]

I like the idea to improve the performance, bug #40 (comment) is not acceptable.