Skip to content

Support storing shallow objects as part of the deepsignal#38

Merged
luisherranz merged 14 commits into
mainfrom
shallow
Jan 31, 2024
Merged

Support storing shallow objects as part of the deepsignal#38
luisherranz merged 14 commits into
mainfrom
shallow

Conversation

@luisherranz

@luisherranz luisherranz commented Jul 24, 2023

Copy link
Copy Markdown
Owner

What

Fixes #36.

Support storing shallow objects as part of the deepsignal with shallow.

It accepts this signatures:

import { deepSignal, shallow } from "deepsignal";

const shallowObj = { a: 1 };
const store = deepSignal({ b: 2, shallowObj: shallow(shallowObj) });
import { deepSignal, shallow } from "deepsignal";

const shallowObj = { a: 1 };
shallow(shallowObj);
const store = deepSignal({ b: 2, shallowObj });
import { deepSignal, shallow } from "deepsignal";

const store = deepSignal({ b: 2, shallowObj: { a: 1 } });
shallow(store, "shallowObj");

Why

To prevent deepSignal from deep observing objects that don't need to be observed.

How

By exposing a shallow function that adds references to the ignore set used by the shouldProxy function.

@luisherranz luisherranz self-assigned this Jul 24, 2023
@luisherranz luisherranz mentioned this pull request Jul 24, 2023
Closed
@github-actions

github-actions Bot commented Jul 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Size Change: +109 B (+2%)

Total Size: 6.49 kB

Filename Size Change
packages/deepsignal/core/dist/deepsignal-core.js 1.05 kB +15 B (+1%)
packages/deepsignal/core/dist/deepsignal-core.mjs 1.05 kB +22 B (+2%)
packages/deepsignal/dist/deepsignal.js 1.09 kB +14 B (+1%)
packages/deepsignal/dist/deepsignal.mjs 1.09 kB +23 B (+2%)
packages/deepsignal/react/dist/deepsignal-react.js 1.11 kB +14 B (+1%)
packages/deepsignal/react/dist/deepsignal-react.mjs 1.11 kB +21 B (+2%)

compressed-size-action

@luisherranz

luisherranz commented Jul 24, 2023

Copy link
Copy Markdown
Owner Author

@wittjosiah, I haven't figured out the TypeScript part yet, but I've released this with a different tag so you can test it out and let me know what you think:

https://www.npmjs.com/package/deepsignal/v/1.4.0-shallow.0

npm install deepsignal@shallow

wittjosiah
wittjosiah approved these changes Jul 25, 2023
View reviewed changes

@wittjosiah wittjosiah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@luisherranz works great on our end from my initial test. If we run into any issues with it I'll let you know and I'll keep an eye out for this to come in a release. Thanks for tackling this 🙂

@luisherranz

luisherranz commented Jul 26, 2023

Copy link
Copy Markdown
Owner Author

Thanks @wittjosiah.

rozek added a commit to rozek/deepsignal that referenced this pull request Sep 20, 2023
@rozek
changes from pull request luisherranz#38
377d7ac
@rozek

rozek commented Sep 20, 2023

Copy link
Copy Markdown

I have some concerns about the code in this pull request: the shallow function does not remove any already existing signals for the given object - which could exist after "deepSignal"ling a class instance (my code currently crashes because of such a situation)...

@rozek

rozek commented Sep 20, 2023

Copy link
Copy Markdown

Well, I might have misunderstood the "shallowing" mechanism: it seems to ignore complete objects only - not just properties.

Even in the case shallow(object,key) it is the current object referred to by object.key which is ignored. As soon as a different object is assigned to object.key, it will be deeply observed again...

This was referenced Sep 20, 2023
Merged
Closed
@oravecz

oravecz commented Sep 24, 2023

Copy link
Copy Markdown

When you mention "I haven't figured out the TypeScript part yet", does this refer to the type returned by shallow() being <T> and state expects the assignment to be <DeepSignalObject<T>>?

This is what I see, but I'm not sure I am implementing it correctly.

store.state.categoryLookup = shallow(
  new CategoryLookup(categories),
) as DeepSignal<CategoryLookup>;  // Have to cast as shallow is returning <CategoryLookup>

Perhaps I am setting the state's property incorrectly, since it is an object versus a primitive?

@luisherranz

luisherranz commented Sep 25, 2023

Copy link
Copy Markdown
Owner Author

the shallow function does not remove any already existing signals for the given object

Yes, that's it. You need to use shallow before adding that property to the state.

it seems to ignore complete objects only - not just properties

Yes, that's how it works and hence the name "shallow": it will only observe the reference to that object, but not the object itself.

Have to cast as shallow is returning <CategoryLookup>

It's not a DeepSignal. So in that case, I believe CategoryLookup should be correct, isn't it?

@github-actions github-actions Bot mentioned this pull request Sep 29, 2023
Merged
4 tasks
luisherranz
luisherranz commented Jan 15, 2024
View reviewed changes
Comment thread .changeset/pre.json
@@ -0,0 +1,10 @@
{

@luisherranz luisherranz Jan 15, 2024

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This file will be removed once we publish the next version, no need to take care of it manually.

Comment thread README.md Outdated
@luisherranz luisherranz marked this pull request as ready for review January 15, 2024 11:54
@luisherranz luisherranz requested a review from DAreRodz January 15, 2024 11:54
@luisherranz

luisherranz commented Jan 15, 2024

Copy link
Copy Markdown
Owner Author

I'm happy with TypeScript now. I've also removed the shallow(store, "shallowObj") API and only shallow(shallowObj) will be officially supported.

DAreRodz
DAreRodz approved these changes Jan 30, 2024
View reviewed changes

@DAreRodz DAreRodz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great, @luisherranz! 👏

PS: I've only seen a couple of odd TS tests.

Comment thread packages/deepsignal/core/test/types.ts Outdated
Comment thread packages/deepsignal/core/test/types.ts Outdated
@luisherranz luisherranz merged commit 8e7875c into main Jan 31, 2024
@luisherranz luisherranz deleted the shallow branch January 31, 2024 12:48
@github-actions github-actions Bot mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DAreRodz DAreRodz DAreRodz approved these changes

+1 more reviewer

@wittjosiah wittjosiah wittjosiah approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@luisherranz luisherranz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stop deepSignal from proxying certain properties or classes

5 participants

@luisherranz @rozek @oravecz @wittjosiah @DAreRodz