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

Feat/solid bindings #88

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bigmistqke
Copy link

@bigmistqke bigmistqke commented Dec 14, 2022

this PR adds solid-bindings to SyncedStore.
The PR is quite minimal: Solid's createMutable proved to be an excellent candidate for wiring SyncedStore's reactivity with Solid.

Besides that I introduced an extra proxy-trap (i think, not sure about the terminology) in the array-getter.

Instead of mapping (like react), solid has control flow-components to deal with arrays (and objects with solid-primitive's Entries):

a react'sstore.todos.map(todo => todo.title) would be idiomatically written in solid like <For each={store.todos}>{todo => todo.title}</For>. <For each={store.todos}> was currently not working, as store.todos returned a reference to the Yjs-object (I assume) instead of the array of proxies. A spread was needed <For each={[...store.todos]}> which I believe would be cause of confusion and bugs when being used in a solid codebase.

An extra proxy-trap is introduced to allow an idiomatic use of arrays and objects in Solid's control flow components: in the Proxy-getter of array we check if the array is being accessed with a symbol that isn't one of SyncedStore's if (typeof p === "symbol" && p !== $reactiveproxy && p !== $reactive && p !== $skipreactive). If this is the case we return a map of the YjsReturnValues.

With this proxy-trap SyncedStore's store behaves very similar to createMutable (besides the fact that you can not set arrays directly in SyncedStore's store) and all control flow-components behave as you would expect.

I am unsure if this proxy-trap has any implications for other frameworks.

bigmistqke and others added 4 commits December 14, 2022 17:06
use solid/store.createMutable to wire up reactivity
to enable idiomatic use of objects and arrays in solid's Flow-Components
an array of parseYjsReturnValue is returned when the object is accessed
with a symbol that is not $reactiveproxy, $reactive or $skipreactive.
@YousefED
Copy link
Owner

@bigmistqke I'd love to merge this, but my knowledge of solidjs is limited. I'm trying to add a unit test to see if we can test whether nested updates trigger effects (see my latest commit). I'm probably doing something wrong when bootstrapping Solid unit tests. Would you have some time to have a look?

I now also commented out your Proxy trap, I'd like to test this later by creating a separate unit test with a For-construct (but we should first get a basic unit test working).

Thanks again for your contribution

@bigmistqke bigmistqke marked this pull request as draft February 1, 2023 18:39
@bigmistqke
Copy link
Author

bigmistqke commented Feb 1, 2023

looking back at this pr... it really shouldn't be merged yet, was too optimistic lol.
I'll set the PR to draft.

  1. Solid assumes we are in SSR-mode, since we are running on the server. Solid deactivates reactivity on the server for performance-reasons. There is currently not yet a DX-friendly way to have reactivity working on the server, the only way to go around it is to link directly to the browser-version of solid.
  2. const implicitStore1 = solid.createMutable(store); is this a common pattern? I don't think currently it is possible to wrap a syncedstore inside a createMutable, because of conflicting proxy-traps. I was under the assumption you would mutate the store directly?
  3. createEffect always runs when it is initialized, but this call is queued, after that it synchronously gets called when a reactive value is updated. playground to illustrate
  4. I updated the test, but it fails: there is no effect-call when updating the store. This is possibly a bug in my implementation. I also get the infamous Yjs was already imported. This breaks constructor checks and will lead to issues!-error.

@YousefED
Copy link
Owner

YousefED commented Feb 1, 2023

Alright!

  1. This was just me trying to try getting the binding working, don't think it's necessary

  2. maybe try cleaning node_modules (also in packages/core), make sure you're on node 16 (nvm use if using nvm) , and use npm install to install again. Or try a clean checkout?

@edemaine
Copy link

edemaine commented Feb 1, 2023

Regarding 1: Apparently Solid will soon support import ... from "@solidjs/reactivity", even server-side. I'm not sure whether it works now or whether this is a 2.0 thing, but at least it's coming.

@bigmistqke
Copy link
Author

that is very exciting news, thanks for sharing.

@bigmistqke
Copy link
Author

bigmistqke commented Feb 1, 2023

maybe try cleaning node_modules (also in packages/core), make sure you're on node 16 (nvm use if using nvm) , and use npm install to install again. Or try a clean checkout?

Just reinstalled with npm instead of pnpm and that did the job 😊 the updated test is passing now.

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.

None yet

3 participants