Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't wrap proxies in proxies #20

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Don't wrap proxies in proxies #20

merged 3 commits into from
Jun 21, 2023

Conversation

luisherranz
Copy link
Owner

What

Don't wrap proxies in proxies.

Why

Because if so, copying an object creates an internal derivation of its primitive values when the signals of those values are recreated.

How

Just check if the target is a known proxy or not, and if it is, just use it as the value.

@luisherranz luisherranz self-assigned this Jun 15, 2023
@luisherranz luisherranz linked an issue Jun 15, 2023 that may be closed by this pull request
@luisherranz luisherranz force-pushed the dont-reproxy-proxies branch from aefd717 to a945b8a Compare June 15, 2023 11:11
@github-actions
Copy link
Contributor

Size Change: +159 B (+3%)

Total Size: 5.47 kB

Filename Size Change
packages/deepsignal/core/dist/deepsignal-core.js 890 B +29 B (+3%)
packages/deepsignal/core/dist/deepsignal-core.mjs 880 B +27 B (+3%)
packages/deepsignal/dist/deepsignal.js 930 B +27 B (+3%)
packages/deepsignal/dist/deepsignal.mjs 923 B +25 B (+3%)
packages/deepsignal/react/dist/deepsignal-react.js 929 B +27 B (+3%)
packages/deepsignal/react/dist/deepsignal-react.mjs 922 B +24 B (+3%)

compressed-size-action

@luisherranz luisherranz mentioned this pull request Jun 15, 2023
@luisherranz luisherranz requested a review from DAreRodz June 15, 2023 11:18
@luisherranz
Copy link
Owner Author

@DAreRodz, would you mind taking a look at this change as well?

@bigmistqke
Copy link

bigmistqke commented Jun 20, 2023

I would copy the reference/signal instead of reproxying/duplicating the value, since this mirrors more closely the API of a regular js-object.

example:

const obj = {x: undefined, y: {id: 100}};
obj.x = obj.y;
obj.y.id++;
console.log(obj.x); // {id: 101}

so with deepSignal it would be

const obj = deepSignal({x: undefined, y: {id: 100}});
obj.x = obj.y;
obj.y.id++;
console.log(obj.x); // {id: 101}

and if I would want to re-proxy I could do

const obj = deepSignal({x: undefined, y: {id: 100}});
obj.x = obj.$y.value;
obj.y.id++;
console.log(obj.x); // {id: 100}

or introduce an unwrap-function (see solid)

const obj = deepSignal({x: undefined, y: {id: 100}});
obj.x = unwrap(obj.y);
obj.y.id++;
console.log(obj.x); // {id: 100}

@luisherranz
Copy link
Owner Author

I would copy the reference/signal instead of reproxying/duplicating the value, since this mirrors more closely the API of a regular js-object.

example:

const obj = {x: undefined, y: {id: 100}};
obj.x = obj.y;
obj.y.id++;
console.log(obj.x); // {id: 101}

so with deepSignal it would be

const obj = deepSignal({x: undefined, y: {id: 100}});
obj.x = obj.y;
obj.y.id++;
console.log(obj.x); // {id: 101}

Yeah, that's how it will work after this PR is merged. Proxies and targets are linked, so reusing the proxy means reusing the target.

@bigmistqke
Copy link

aa sweet! misread the code!

Copy link
Collaborator

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, thanks for the fix @luisherranz 😁

@luisherranz luisherranz merged commit b619170 into main Jun 21, 2023
@luisherranz luisherranz deleted the dont-reproxy-proxies branch June 21, 2023 13:05
@github-actions github-actions bot mentioned this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overwrapping proxies
3 participants