Slimmed deep proxy #23
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
So, the question is whether there's obvious bugs... |
dai-shi
commented
May 7, 2019
src/utils.js
Outdated
| if (origObj === nextObj) return false; | ||
| if (typeof origObj !== 'object') return true; | ||
| if (typeof nextObj !== 'object') return true; | ||
| if (!affected.has(origObj)) return false; // is this safe??? |
There was a problem hiding this comment.
This one is the most curious. It behaves differently from proxyEqual.
🚀 🚀 🚀 |
dai-shi
commented
May 8, 2019
src/utils.js
Outdated
| if (typeof origObj !== 'object') return true; | ||
| if (typeof nextObj !== 'object') return true; | ||
| if (!affected.has(origObj)) { | ||
| return depth !== 0; // false for root object, but true for others |
There was a problem hiding this comment.
I'm wondering if this returns false always, we don't need to care about "rest" operator.
I really need help from @theKashey with this stuff...
There was a problem hiding this comment.
That might be needed in other cases. What if you Object.keys(props)?
There was a problem hiding this comment.
Oh, it's Symbol(Symbol.iterator). I'm not sure about other cases, but what I could do is to assume the whole object is affected if accessed by a symbol property. (I hesitate writing my own proxy, when I understand that I don't really understand fully...)
(edit) it was not Symbol.iterator, but ownKeys handler. I don't know why I misunderstood that.
There was a problem hiding this comment.
And, it's needed for reference equality checking too.
Could we ignore for the root object? Do people do this?
const state = useReduxState();
return useMemo(() => ({ foo: 'foo' }), [state]);Yeah, maybe?
theKashey
reviewed
May 8, 2019
src/utils.js
Outdated
| } | ||
| const changed = affected.get(origObj) | ||
| .some(key => isDeepChanged(origObj[key], nextObj[key], affected, cache, depth + 1)); | ||
| cache.set(origObj, { nextObj, changed }); |
There was a problem hiding this comment.
To break cycles you have to set cache(to undefined) before diving deeper.
There was a problem hiding this comment.
Do you mean cycles in state object tree? I need to understand the problem by writing test code...
theKashey
reviewed
May 8, 2019
src/utils.js
Outdated
| if (typeof val !== 'object') { | ||
| return val; | ||
| } | ||
| if (!proxyMap[key]) { |
There was a problem hiding this comment.
proxy map should be external to deepProxy to keep the same req equality between calls, or you will get always unique objects each render.
There was a problem hiding this comment.
Totally! I keep forgetting about the ref equality.
There was a problem hiding this comment.
And it is not trivial. In the end, I'd reinvent proxyEqual, which I really don't want to do.
theKashey
reviewed
May 8, 2019
src/utils.js
Outdated
| if (!used.includes(key)) used.push(key); | ||
| } | ||
| const val = target[key]; | ||
| if (typeof val !== 'object') { |
There was a problem hiding this comment.
there is a few objects (like buitins) which could not be proxied. Plus there are "frozen" objects :(
There was a problem hiding this comment.
I see. I should somehow detect it and work it around. (Or, maybe just warn developers.)
theKashey
reviewed
May 8, 2019
src/utils.js
Outdated
| affected.set(target, [key]); | ||
| } else { | ||
| const used = affected.get(target); | ||
| if (!used.includes(key)) used.push(key); |
There was a problem hiding this comment.
This turns out to be "really" slow when used is big.
There was a problem hiding this comment.
Have you tried Set?
Probably it's time to use some V8-level hacks, like a using different approaches and data structures depending on "size".
theKashey
reviewed
May 8, 2019
src/utils.js
Outdated
| } | ||
| } | ||
| const changed = affected.get(origObj) | ||
| .some(key => isDeepChanged(origObj[key], nextObj[key], affected, cache, depth + 1)); |
There was a problem hiding this comment.
I'll propose use for loop here, with early break in case of a change.
There was a problem hiding this comment.
some does early break though. Anyway, the for loop is often faster, so I will try when we optimize it (after making sure it works correctly.)
|
Looking great, but I urge you to hold it until comprehensive tests written. It took me a while to fix all bugs in |
|
@theKashey Thanks so much for your comments. I will write some tests. Honestly, it's hard for me to tell if they are comprehensive or not. |
|
@theKashey I did what I can do at this point. If you have time, your comments are so appreciated. |
Benchmark with useReduxState 69023f9 |
theKashey
reviewed
May 12, 2019
package.json
Outdated
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "reactive-react-redux", | |||
| "description": "React Redux binding with React Hooks and Proxy", | |||
| "version": "2.0.1", | |||
| "version": "3.0.0-alpha.0", | |||
Benchmark with useReduxState 8373b97 |
|
If I understand it correctly, it means to use a global diff --git a/src/useReduxState.js b/src/useReduxState.js
index 8d01436..888173c 100644
--- a/src/useReduxState.js
+++ b/src/useReduxState.js
@@ -69,12 +69,12 @@ export const useReduxStateRich = () => {
return trapped.state;
};
+const proxyCache = new WeakMap();
export const useReduxState = (opts = {}) => {
const forceUpdate = useForceUpdate();
const store = useContext(ReduxStoreContext);
const state = store.getState();
const affected = new WeakMap();
- const proxyCache = useRef(new WeakMap());
const lastTracked = useRef(null);
useIsomorphicLayoutEffect(() => {
lastTracked.current = {
@@ -109,7 +109,7 @@ export const useReduxState = (opts = {}) => {
const unsubscribe = store.subscribe(callback);
return unsubscribe;
}, [store]); // eslint-disable-line react-hooks/exhaustive-deps
- return createDeepProxy(state, affected, proxyCache.current);
+ return createDeepProxy(state, affected, proxyCache);
};
export const useReduxStateSimple = () => {Benchmark: baseline with 6a5879bBenchmark: with the patchA bit better. (but, not that drastically?) Another pros is keeping ref equality among hooks, so: const MyComponent = () => {
const state1 = useReduxState();
const state2 = useReduxState();
// state1 === state2
...The cons is the tracking may not be accurate: const ParentComponent = () => {
const state = useReduxState();
const foo = state.foo;
return <ChildComponent foo={foo} />;
};
const ChildComponent = ({ foo }) => {
const state = useReduxState();
const foo2 = state.foo;
return (
<>
<Foo foo={foo2} /> 👉 foo2 is tracked in ChildComponent
<Foo foo={foo} /> 👉 foo is also tracked in ChildComponent although it's from ParentComponent
</>
);
};I don't know what would happen if there're async calls. |
const Parent = () => {
const state = useReduxState();
const foo = state.foo;
return (
<>
<Child1 />
<Child2 foo={foo} />
</>
);
};
const Child1 = () => {
const state = useReduxState();
const foo = state.foo;
return (
<>
<Foo foo={foo} /> 👉 foo is tracked in Child1
</>
);
};
const Child2 = ({ foo }) => {
return (
<>
<Foo foo={foo} /> 👉 foo is tracked as used in Child1. 😵
</>
);
};I thought it would be problematic in concurrent mode, but even in non-concurrent mode, it may not track as expected. |
|
Yeah. I was thinking about patching |
|
Interesting. Although, with |
No, this is something you don't want to do const Parent => ({data}) => (
<>
<Child1 data={data} />
<Child2 data={data} />
</>
)
const Child1 = ({data}) => data.stringValue;
const Child2 = ({data}) => data.stringValue[0];💥Parent and Childs would be updated only when first - Adding |
|
Oh no. How do you deal with it in the first place? In your example, the string value is not so problematic because we don't track strings. const Parent = () => {
const state = useReduxState();
const { foo } = state;
return (
<>
<Child1 foo={foo} />
<Child2 foo={foo} />
</>
);
};
const Child1 = ({ foo }) => (
<div>{foo.stringValue}</div>
);
const Child2 = ({ foo }) => useMemo(() => (
<div>{foo.numberValue}</div>
), [foo]);
// Or same thing with React.memo with the second argumentI have no idea how to solve this in userland. 🤔 any workaround... |
|
Let me merge this and I will release it as a new version, hoping someone would try it and give us a feedback. |
|
Everything is looking good (at least yet 🤓). Look like you nearly fixes a new patterns in React, so many people were struggling with. |
|
👍
Could you elaborate this? what do you mean by a new patterns? |
|
Sorry, 🤯🥴💩. I was trying to point on “patterns” commonly used to wire the app - in your case it’s closer to the magic of Vue (which is also backed by proxies). There was a long standing issue with redux and selectors, which is just a now issue for you. So you are fixing old patterns by a new ones. This is something I wanted to say ;) |
|
Got it! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From: theKashey/proxyequal#17 (comment)
I feel like I'd write a slimmed version of deep proxy.