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

restore: Auto-clean old blobs from IndexedDB #369

Merged
merged 17 commits into from
Oct 3, 2017

Conversation

goto-bus-stop
Copy link
Contributor

@goto-bus-stop goto-bus-stop commented Sep 28, 2017

Steps:

  • Add expiry timestamp to blobs in IndexedDB
  • Implement cleaning up expired blobs from IndexedDB
  • Auto-cleanup when a RestoreFiles plugin is installed
  • Remove Core dependency from IndexedDBStore and ServiceWorkerStore
    Note that this removed the need for users to fire the sw-ready event, the ServiceWorkerStore now detects whether a serviceWorker is installed on its own.
  • Add expiry to ServiceWorkerStore? Don't know if this is necessary since browsers will probably restart the serviceworker at some point and lose all the blobs before we could hope to expire anything.
  • Add expiry to file metadata in localStorage
  • Add a module that can be used to auto-cleanup Uppy's old blobs without including all of Uppy, so people can run this on every page of their website or whatever at a much smaller byte cost.

@goto-bus-stop goto-bus-stop changed the title restore: Auto-clean old blobs from IndexedDB [wip] restore: Auto-clean old blobs from IndexedDB Sep 28, 2017
@goto-bus-stop
Copy link
Contributor Author

Added require('uppy/lib/plugins/RestoreFiles/cleanup'), a function that you can call to clean up expired file blobs and metadata without importing Uppy itself.

It's not super tiny unfortunately, about 5kB after gzip:

$ browserify src/plugins/RestoreFiles/cleanup.js -t babelify | uglifyjs -cm | gzip | wc -c | npx pretty-bytes-cli
4.8 kB

@goto-bus-stop goto-bus-stop changed the title [wip] restore: Auto-clean old blobs from IndexedDB restore: Auto-clean old blobs from IndexedDB Oct 2, 2017
@goto-bus-stop goto-bus-stop requested a review from arturi October 2, 2017 09:17
sendMessageToAllClients({
type: 'uppy/HERE_I_AM'
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MDN said something about using clients.get(event.clientId) to only send the message back to the sender, but clientId was undefined for me--i guess that's why we were sending the uppy/ALL_FILES message to all clients too.

@arturi
Copy link
Contributor

arturi commented Oct 3, 2017

Got this in Safari on boot:

screen shot 2017-10-02 at 11 36 48 pm

On adding file:

screen shot 2017-10-02 at 11 53 34 pm

Later after refresh:

screen shot 2017-10-03 at 12 13 05 am

Could it be that it’s not expecting that IndexedDB database might not exist? 🤔 Just a guess.

@arturi
Copy link
Contributor

arturi commented Oct 3, 2017

In docs, we are using both RestoreFiles and GoldenRetriever now, when referencing the lib/require path. We should standardize. Let’s use GoldenRetriever everywhere? Or do you feel strongly about keeping RestoreFiles name?

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Oct 3, 2017

Could it be that it’s not expecting that IndexedDB database might not exist? 🤔 Just a guess.

Ah good catch! yeah that's most likely it.

e; it was the database migration (adding timestamps to existing blobs) failing if there were no blobs stored. fixed in 68d533e

In docs, we are using both RestoreFiles and GoldenRetriever now, when referencing the lib/require path. We should standardize. Or do you feel strongly about keeping RestoreFiles name?

hmm, the reason i use RestoreFiles in docs is because that's the path you have to require()…so i don't feel strongly about the name itself, but it should be the same as what it's called in the code

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Oct 3, 2017

Rebased, the only new commits are b62ab29 and fdf46b5 (changelog)

Copy link
Contributor

@arturi arturi left a comment

Choose a reason for hiding this comment

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

Woot woot! Very impressive! Tested, clean up succeeded after recent updates ✔️

A thought about serviceWorker: I think it might stick around forever if you don’t restart the browser, and on mobile its literally forever, because that’s how it’s designed to work. So I think adding expiration dates to serviceWorker is a good idea too.

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