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

Support IndexedDB base URL storage #677

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

geekypradip
Copy link

This PR can Be help with this issue

I've implemented a new feature in this PR. Previously, I encountered issues when uploading more than 30,000 files at once. The problem stemmed from local storage limitations, causing it to fill up quickly and ultimately crash the browser. To address this, I integrated indexDB-based storage into the system. Now, the application checks whether the client's browser supports indexDB. If it does, the system utilizes indexDB; otherwise, it falls back to local storage or noopStorage.

@geekypradip
Copy link
Author

@Acconut

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you very much for this massive PR and congrats on figuring out the internals of tus-js-client. I haven't worked with IndexedDB on my own, so this API is new to me, but the PR is looking promising. I have left a few comments, but they mostly focus on smaller aspects.

On bigger task is adding proper tests. We have localStorage-specific tests in https://github.com/tus/tus-js-client/blob/9cc79b5e530d93fbaff1fe78af2e2cd99165b4b8/test/spec/test-browser-specific.js/#L79 and they might serve as a basis for additional tests for this PR. In these localStorage tests, we access localStorage directly to assert that the correct data has been written to it. However, I am not sure if that would be a good approach for IndexedDB as well here as the code would become verbose and I am unsure if a database can opened multiple times in parallel. Maybe it is better to construct a new IndexedDB URL storage, pass it to tus-js-client and at the end of the test, we can use the findAllUploads method to assert that the correct data has been inserted. Can you give this a try please?

Once we have addresses the above points and have decided on a stable API, we still need to at documentation. But this can be done as the last task.

lib/browser/index.js Outdated Show resolved Hide resolved
lib/browser/index.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
lib/browser/index.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
lib/browser/urlStorageLocalStorage.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
lib/browser/urlStorageIndexDB.js Outdated Show resolved Hide resolved
@geekypradip
Copy link
Author

Thank you @Acconut for your review. I have made the requested changes and tested them in my local project setup where everything is working as expected. However, I am still in the process of writing unit tests for these changes as this is a new area for me. I appreciate your understanding and patience as I work on this. If you are satisfied with the current changes and they meet the project requirements, feel free to merge them. I will continue to work on the tests and submit them as soon as they are ready.

@geekypradip geekypradip changed the title Support IndexDB base URL storage Support IndexedDB base URL storage Apr 4, 2024
@geekypradip geekypradip requested a review from Acconut April 8, 2024 05:50
@AntonOfTheWoods
Copy link

@geekypradip , how are the tests going?

@@ -0,0 +1,112 @@
const isSupportIndexedDB = () => {
return 'indexedDB' in window

Choose a reason for hiding this comment

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

Is there some reason not to use globalThis rather than window here?

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.

3 participants