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

Store is triggered when render is called #256

Closed
valterkraemer opened this issue Jul 30, 2017 · 7 comments
Closed

Store is triggered when render is called #256

valterkraemer opened this issue Jul 30, 2017 · 7 comments
Labels

Comments

@valterkraemer
Copy link
Contributor

I modified the localstorage backend to update Mavo when there is an change in localstorage (f92378d) so that windows/tabs are kept in sync.

window.addEventListener("storage", e => {
    if (e.key !== this.key) {
        return;
    }

    var newValue;

    try {
        newValue = JSON.parse(e.newValue);
    } catch (e) {
        newValue = {};
    }

    this.mavo.render(newValue);
});

This however triggers an unnecessary update. It works fine for localstorage, but for my firebase backend it causes problems.

Or is there a better way of pushing data from backend?

@LeaVerou
Copy link
Member

LeaVerou commented Jul 30, 2017

In general storage-sent data updates after rendering are a little problematic currently: if you were interacting with the app, especially editing data, you could lose a lot of state. Mavo does try to minimize this a bit, by re-rendering on existing elements whenever possible, and only if values have changed, but that doesn't work very well with collections, if items have been added in the middle of the collection.

Could you elaborate on what you mean by "an unnecessary update"? What kind of update?
The main issue I see with this code is that it looks like it be triggered every time you save even via the same app, perhaps this is what you meant?

Also, are you using a custom firebase backend or https://github.com/lizziew/mavo-firebase ?

@valterkraemer
Copy link
Contributor Author

valterkraemer commented Jul 30, 2017

For my use case it's fine to lose edits.

I forgot to mention that the "unnecessary update" happens when mv-autosave="0" is set. Then when calling render autosave calls store. Is there a way of disabling that?

The mavo-firebase plugin didn't meet my needs so I'm working on my own.

@LeaVerou
Copy link
Member

LeaVerou commented Jul 30, 2017

This is a hack, but off the top of my head:

Store the value of mavo.permissions.save, then set it to false (mavo here being the Mavo instance). This will prevent store from being called. Then call mavo.setUnsavedChanges(false); to set the unsavedChanges flag of all nodes to false, then restore the value of mavo.permissions.save.

Eventually, this should be fixed in Mavo. I think it's a bug that calling render() can trigger saving. I cannot think of a single case where that behavior is desirable. Can you?

The mavo-firebase plugin didn't meet my needs so I'm working on my own.

The mavo-firebase plugin is very much a work in progress, @lizziew plans to develop it much further, and I am (loosely) supervising her work. One thing I’m really excited about is adding granular permissions, so that users can have different permissions per Mavo node, since Firebase supports that kind of granularity. Another thing is server-sent updates. Are you sure you don't want to collaborate on making it better instead of making a new one?

@valterkraemer
Copy link
Contributor Author

I tried the hack, but for some reason it still always called store the first time render was called. I ended up checking unsavedChanges in the put function, and that seems to work.

if (!this.mavo.unsavedChanges) {
  return Promise.resolve(serialized);
}

Eventually, this should be fixed in Mavo. I think it's a bug that calling render() can trigger saving. I cannot think of a single case where that behavior is desirable. Can you?

No, I don't see a reason why render should trigger saving.

The mavo-firebase plugin is very much a work in progress, @lizziew plans to develop it much further, and I am (loosely) supervising her work. One thing I’m really excited about is adding granular permissions, so that users can have different permissions per Mavo node, since Firebase supports that kind of granularity. Another thing is server-sent updates. Are you sure you don't want to collaborate on making it better instead of making a new one?

Great to hear of the future plans! The project looked abandoned. At this moment I will however continue on my own path, but I don't see a reason why they couldn't be merged at a later point.

@lizziew: give me a shout when you are planning to continue working on it, and I will rethink my stance.

@LeaVerou
Copy link
Member

I tried the hack, but for some reason it still always called store the first time render was called. I ended up checking unsavedChanges in the put function, and that seems to work.

Hmmm, odd. In any case, your solution is actually better.

No, I don't see a reason why render should trigger saving.

Ok, I will mark this as a bug then!

The project looked abandoned.

@lizziew is a CS major at MIT, I believe she's doing an internship this summer, which is why she is not currently working on it. She will pick it up again in September. Do you plan to publish yours @valterkraemer or is it only for internal use?

@LeaVerou LeaVerou added the bug label Jul 31, 2017
@valterkraemer
Copy link
Contributor Author

Cool.

Do you plan to publish yours @valterkraemer or is it only for internal use?

Planning to publish everything under MIT licence.

@LeaVerou
Copy link
Member

I cannot reproduce this anymore, so I'm closing this. If that's in error, feel free to reopen!

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

No branches or pull requests

2 participants