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

Svelte Support #24

Closed
wants to merge 8 commits into from
Closed

Svelte Support #24

wants to merge 8 commits into from

Conversation

zsaquarian
Copy link

This PR adds support for svelte. Unfortunately, because svelte reactivity is based on assignments, we need a dummy function. Other than that, usage is identical. We could also use svelte stores, but that would need a rewrite of the entire library as the crdt object would need to become a store.

@YousefED
Copy link
Owner

Awesome stuff @zsaquarian !

If I understand the refresh function correctly, I think it would mean that the app refreshes whenever any data inside SyncedStore changes, correct?

Ideally (this is similar to how the React / Vue bindings work), a refresh would only be triggered when the actually used (subscribed) data changes.

I suppose this is what you mean why we'd need to use Svelte Stores, right?

Would it be possible to create a store for every "atom" in useSvelteBindings, or is that not how Svelte stores work (I'd have to dive in more deeply to see if this suggetion makes sense)?

@zsaquarian
Copy link
Author

zsaquarian commented Nov 12, 2021

Awesome stuff @zsaquarian!

Thanks! :)

If I understand the refresh function correctly, I think it would mean that the app refreshes whenever any data inside SyncedStore changes, correct?

Yes

Ideally (this is similar to how the React / Vue bindings work), a refresh would only be triggered when the actually used (subscribed) data changes.

I didn't really understand what you mean by subscribed data vs. any data.
Is it something like this?

The store has a todos field and also other field. We only use store.todos, but the component rerenders even when store.other is changed.

If so, then it doesn't. I just checked it.

I suppose this is what you mean why we'd need to use Svelte Stores, right?

Perhaps I should have been clearer. refreshis just a function which assigns a variable to itself. (This doesn't refresh the page, it instead forcefully rerenders the component. Maybe I should change the name?). This is because svelte doesn't watch for changes in variables, but rather assignments to that variable. According to the docs:

Not all application state belongs inside your application's component hierarchy. Sometimes, you'll have values that need to be accessed by multiple unrelated components, or by a **regular JavaScript module.**

In Svelte, we do this with stores.

Would it be possible to create a store for every "atom" in useSvelteBindings, or is that not how Svelte stores work (I'd have to dive in more deeply to see if this suggetion makes sense)?

I tried that originally, but it seems svelte will only rerender a component if the store is directly used in that component. This basically means that the crdt type would have to become a store.

@YousefED
Copy link
Owner

The store has a todos field and also other field. We only use store.todos, but the component rerenders even when store.other is changed.

Yes, this is what I meant. If it doesn't rerender now, I think that's because there's no other component using store.other. My suspicion is that if you'd have two components (one using todos, and the other one using other), both would rerender if any of the values change. Could you confirm that?

@zsaquarian
Copy link
Author

Yes, it seems like both of them rerender when only one is changed
issue

@YousefED
Copy link
Owner

I think we can make this work using the Reactive library. Could you try something like this? (I haven't tested it yet)

import { readable } from 'svelte/store';
import { Observer, reactive } from "@reactivedata/reactive";

const svelteSyncedStore = (syncedStore: any) => {
    let set: any;
    const observer = new Observer(() => {
        if (set) {
            set(store);
        }
    });
    const store = reactive(syncedStore, observer);
    
    return readable(store, newSet => {
        set = newSet;
        
        return () => { set = undefined};
    });
};

In your component:

// where syncedStore would be the return value of the normal syncedStore({ todos: [] }) setup
const store = svelteSyncedStore(syncedStore); 
...

// now use $store
$store.todos[0].completed

@zsaquarian
Copy link
Author

Unfortunately no :(

Even if we make it a writable store (so we can modify it), we would still have to do something like this:

$store.todos = [...$store.todos, newItem];

Which throws Uncaught Error: cannot set new elements on root doc.
I don't think stores will do what we want them to do as this issue says that updating a store will trigger a rerender for all components using that store. :(

Also, I don't really see the problem with rerendering a component? Am I missing something?

@YousefED
Copy link
Owner

Which throws Uncaught Error: cannot set new elements on root doc.

That error is expected, the correct pattern for that would be $store.todos.push(newItem). Does that work?

Also, I don't really see the problem with rerendering a component? Am I missing something?

Performance wise, you would want to make sure only the relevant component(s) rerender if the synced data change. Especially when multiple developers / projects will depend on the library

@zsaquarian
Copy link
Author

zsaquarian commented Nov 12, 2021

That error is expected, the correct pattern for that would be $store.todos.push(newItem). Does that work?

No, since there is no assignment there.

Performance wise, you would want to make sure only the relevant component(s) rerender if the synced data change. Especially when multiple developers / projects will depend on the library

Ah OK. I understand.

Also, fortunately, I seem to have made it work! Svelte has an afterUpdate method, and I just put a console.log in that method. All I changed is to have the Yjs doc in a separate module and import it into both of them, but not the crdt and useSvelteBindings call. This seems to be working properly now.

2021-11-12.22-37-28.mp4

Edit: the repetition at the top is because I am inserting that text into other. I didn't show it in the video, but that correctly updates only the other and not todos.

@YousefED
Copy link
Owner

Cool! I just gave it a shot as well, see https://github.com/YousefED/SyncedStore/tree/svelte.

What do you think? I'd move svelteSyncedStore to a separate module if we want to support it officially

@zsaquarian
Copy link
Author

Awesome!
I had meant that I had gotten my previous approach to work, but this is much more elegant. :)

What do you think? I'd move svelteSyncedStore to a separate module if we want to support it officially

I agree.

Thanks!

@zsaquarian zsaquarian closed this Nov 12, 2021
@YousefED
Copy link
Owner

This has been published in 0.3.4 and PR #25

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.

2 participants