Repository navigation
Support IndexedDB base URL storage - #677
geekypradip wants to merge 10 commits into
Conversation
Support Index db url storage
Acconut
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Marius <marius.kleidl@gmail.com>
…nto indexDB-url-storage
Index db url storage
|
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 , how are the tests going? |
| @@ -0,0 +1,112 @@ | |||
| const isSupportIndexedDB = () => { | |||
| return 'indexedDB' in window | |||
There was a problem hiding this comment.
Is there some reason not to use globalThis rather than window here?
This PR can Be help with this issue