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

Trigger event on content change in syncservice #2465

Merged

Conversation

vinicius73
Copy link
Member

Summary

Fires 4 events from SyncService

  • sync-service:idle
  • sync-service:save
  • sync-service:error
  • sync-service:sync

Prevention of Memory Leak

All SyncService event handlers are storage in arrays, and never removed from these arrays.
Although the SyncService whore removed from Vue component context, we had a circular reference here.
This is a very common scenario for Memory Leak in JavaScript.

Now using mitt library is possible to use .off to remove an event handler.

To do it, I've change part of event contracts (error event) and I've split the event handlers in component methods.

There are more potential problems to solve, but we can do it later.

@vinicius73 vinicius73 marked this pull request as ready for review May 31, 2022 19:37
@vinicius73
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <[email protected]>
@vinicius73 vinicius73 requested a review from julien-nc June 1, 2022 12:49
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Much cleaner 👍

@vinicius73 vinicius73 merged commit 6ac37c5 into master Jun 2, 2022
@delete-merged-branch delete-merged-branch bot deleted the 2462-trigger-event-on-content-change-in-syncservice branch June 2, 2022 14:23
@max-nextcloud
Copy link
Collaborator

/backport to stable24

@max-nextcloud
Copy link
Collaborator

Not sure if we really want to backport this - but since it prevents memory leaks and cleans things up seems like a good idea.

@backportbot-nextcloud
Copy link

The backport to stable24 failed. Please do this backport manually.

@mejo-
Copy link
Member

mejo- commented Jul 6, 2022

Manual backported to stable24 in #2679

@mejo- mejo- added backported successfully backported and removed backport-request labels Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger event on content change in syncService
5 participants