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

Add option to avoid mutating original data passed to store and its functions #1867

Open
mathieuprog opened this issue Aug 29, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@mathieuprog
Copy link
Contributor

mathieuprog commented Aug 29, 2023

Describe the bug

https://playground.solidjs.com/anonymous/ef7f22ce-7141-4690-8fe3-a5f5497a0ac0

When a store gets updated, it updates the original data as well. In the example above, let's say that dataFromExternalLibrary should not mutate. The solution would be to deep clone the original data. But:

https://playground.solidjs.com/anonymous/987aa07c-26d2-42e0-af9f-b03336ba260f

In the case where we use a store as the underlying storage of createResource, whenever data needs to be reconciled, the user would need to deep clone, if the user wants to avoid mutating the original object.

If an option allows Solid to clone, it would allow for less cloning. In the lack of such option, we need to deep clone the whole data received from e.g. a library (which may be large) every time:

function createDeepSignal<T>(value: T): Signal<T> {
  const [store, setStore] = createStore({ value });

  return [
    () => store.value,
    (v: T) =>
      untrack(() => {
        const unwrapped = unwrap(store.value);
        typeof v === 'function' && (v = v(unwrapped));
        
        // <=== deep clone `v`
        
        setStore('value', reconcile(v));
        return store.value;
      })
  ] as Signal<T>;
}

(My real-world case involves receiving GraphQL cached data, i.e. JS objects held by the GraphQL client library. Those caches should not mutate, hence I need to deep clone).

Platform

  • OS: Windows
  • Browser: Chrome
  • Version: 116

Additional context

https://discord.com/channels/722131463138705510/1126684602480734368/1145982497281282139 (last messages)

@ryansolid
Copy link
Member

I see. This helps very specifically in the case where the data coming in is always new. In those cases referential equality (of non-primitive values) gives you no benefit. Structured clone checks would always fail anyway. So instead of the user cloning the whole new structure upfront before reconciliation, we would clone the parts that would be replaced as we set them which given there is no referential equality would likely be less.

Hmm... yeah this would be something that would benefit a resource-like situation specifically where it's always new and always reconciled. Downside is necessary inclusion of deep cloning logic and all that comes with that in the bundle as it wouldn't be tree shakeable. unwrap does a certain amount of cloning but I think it bails out early which won't do here. I'd usually point this to primitives and userland but there isn't enough access to it. I think this is an interesting topic similar to path-based notification where I'd like to give it some consideration in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants