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

indexeddb-files: example using the IndexedDB API to store and retrieve files #171

Closed
wants to merge 4 commits into from

Conversation

rpl
Copy link
Member

@rpl rpl commented Jan 2, 2017

This add-on shows how to read and save files using the IndexedDB API and the sub-set of the "File and Directory Entries API" supported on Firefox.

@rpl
Copy link
Member Author

rpl commented Jan 2, 2017

@wbamberg This is an example addon with a collection of small examples (well... as small as possible :-)) related to some common use cases around files:

  • reading local files and save them into indexeddb, and then read them from their version stored in the addon
  • creating a new mutable file from scratch scratch, save data into it, persist it and read it from indexeddb

can you take a look at it and help me to make it as clear and helpful as possible? :-)

(as a further step it would be probably even more helpful if we turn it into a howto/tutorial form on MDN, manipulating files efficently is going to be probably a pretty common use case)

@rpl
Copy link
Member Author

rpl commented Jan 3, 2017

@mixedpuppy This is an example related to how to manipulating files from a WebExtension using the existent WebAPIs (in particular the IndexedDB and the subset of the "File and Directory Entries" APIs already implemented in Firefox) that we talked about during the All Hands, can you take a look when you got some time? as usually any ideas/suggestion/advice is welcome ;-)

dbRequest.onsuccess = (evt) => {
const file = evt.target.result;
if (!file) {
dbLog("File ${filename} not found.");
Copy link

Choose a reason for hiding this comment

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

this needs to be a template literal

dbRequest.onsuccess = (evt) => {
const file = evt.target.result;
if (!file) {
dbLog("File ${filename} not found.");
Copy link

Choose a reason for hiding this comment

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

this needs to be a template literal too

@wbamberg
Copy link

wbamberg commented Jan 10, 2017

I didn't check the code in detail for correctness, but read it to make sure I understood what it's doing. It's very nice to read and easy to understand, as far as that's possible with indexedDB :). And it worked perfectly for me.

It's a bit strange that it's doing three different things, especially when one ("reading-local-dirtree") isn't directly related to indexedDB. Since IIUC you're already exercising this API in "indexeddb-save-files", do you need "reading-local-dirtree" at all?

It would seem more natural, maybe, to make "tab.html" (which could have a better name perhaps, like "indexeddb-demo" or something) be a browser action popup. Then you click the popup to see the options, that then open new browser tabs.

File naming:

  • "indexeddb-filehandle" is an odd name for something that everywhere else talks about mutable files. Why not call it "indexeddb-mutablefile"? Or, if you did opt to remove "reading-local-dirtree", you could just call it "mutablefile", since "indexeddb-", should be obvious.

  • indexeddb-save-files" could be "[indexeddb-]save-local-directory" to be more precise (since "indexeddb-filehandle" also saves files :-)).

For the UI:

  • it would help I think if the log were more clearly differentiated from the input buttons and boxes. Maybe it could get a different background color, like a light grey? It would also be great if the log scrolled so the latest output is visible :).

  • "Clear logs" could be after the log, so it's more clearly associated with it

  • having links directly from the example pages to MDN (for IndexedDB and MutableFile) might be worth doing?

  • the hr elements are quite distracting especially in "indexeddb-filehandle". Some margins might group things just as well, without looking so jarring.

  • I did wonder if it would be better to disable buttons until they can be used, to make the flow clearer. But maybe it's not worth the extra code.

  • the UI and docs and stuff sometimes days "persisted" and sometimes "saved". I think "saved" is better.

@@ -0,0 +1,35 @@
# indexeddb-files

This add-on shows how to read and save files using the IndexedDB API and the sub-set of the "File and Directory Entries API" supported on Firefox.
Copy link

@wbamberg wbamberg Jan 10, 2017

Choose a reason for hiding this comment

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

sub-set -> subset (here and below)

## What it shows ##

- **Reading files from a local directory tree**: which shows how to use the sub-set of the "File and Directory API" supported on Firefox to read a directory tree instead of single files
- **Save files into IndexedDB**: which shows how to use the IndexedDB files to save and read files from indexedDB (and how to extract of the files as a single zip files)

Choose a reason for hiding this comment

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

"extract of the files as a single zip files" -> "download the files as a single zip file".

You could also say that this shows how to save the contents of a local directory tree into indexedDB.

* https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API

- IndexedDB MutableFile API:
* https://developer.mozilla.org/en-US/docs/Web/API/IDBMutableFile
Copy link

@wbamberg wbamberg Jan 10, 2017

Choose a reason for hiding this comment

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

It's a shame the MutableFile docs are so poor :(.

<!DOCTYPE html>
<html>
<head>
<title>Files from a WebExtension Examples</title>

Choose a reason for hiding this comment

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

This is an odd name. Perhaps "IndexedDB/file handling demos"?

<h1>Files from a WebExtension</h1>
<ol>
<li><a href="tabs/reading-local-dirtree.html">Reading files from a local directory tree</a></li>
<li><a href="tabs/indexeddb-save-files.html">Save files into IndexedDB</a></li>

Choose a reason for hiding this comment

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

"Save the contents of a local directory using indexedDB"

<ol>
<li><a href="tabs/reading-local-dirtree.html">Reading files from a local directory tree</a></li>
<li><a href="tabs/indexeddb-save-files.html">Save files into IndexedDB</a></li>
<li><a href="tabs/indexeddb-filehandle.html">Create temporary files</a></li>

Choose a reason for hiding this comment

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

"Create temporary files and save them using IndexedDB"

</style>
</head>
<body>
<h1>Create mutable files using the MutableFile API, and then save and retrieve them using IndexedDB API</h1>

Choose a reason for hiding this comment

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

This is quite long! Could we make the H1 "Create and save temporary files" and then have a P underneath giving the details, like "Create a temporary file, give it some data and a name, then save it using indexedDB".


<hr/>

<h1>1. Create temporary files</h1>

Choose a reason for hiding this comment

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

"Create a temporary file"


<div>
<button id="create-file">Create a MutableFile</button>
<button id="get-metadata">Get Metadata of the opened MutableFile</button>

Choose a reason for hiding this comment

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

"Get the MutableFile's metadata"

<div>
<textarea id="file-data" placeholder="data to save into the file..."
style="width: 100%;"></textarea>
<button id="save-data">Save Data into the opened MutableFile</button><br/>
Copy link

@wbamberg wbamberg Jan 10, 2017

Choose a reason for hiding this comment

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

"Save Data into the opened MutableFile" "save" seems strange to me when it hasn't been saved. Could we say "Add data to the MutableFile"?

Or is "saving" (as distinct from "persisting") the correct way to refer to data written to a temp file?

};
});

// Handle the save-data, get-metadata and persist-file buttons.

Choose a reason for hiding this comment

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

This line (IIUC) refers to the following addEventListener calls. It would be better to split it so each one gets a comment.

It could be a bit more extensive too: "Handle the "save-data" button: fetch the contents of the "file-data" textarea and write it to the currently open temporary file."

@rpl
Copy link
Member Author

rpl commented Jan 10, 2017

@wbamberg Thanks a lot for the detailed review, it has been very helpful!

I just pushed a new commit with a bunch of changes based on the review comments:

  • the first demo has been removed as suggested
  • the list of demos is now opened in a browser_action popup window
  • the stylesheet and the logs helper have been moved into shared css and js files
  • the HTML pages related to the two examples have been re-organized and re-formatted (e.g. the logs are now in a column on the right and the logs content is auto-scrolled to the bottom, the steps of the demos are now separated using <details/> tags, instead of the <hr> tags, and these details are expanded when the step is ready to be executed, and the input and button elements enabled accordingly).
  • in the first example, when a saved image file is opened, the images is now added to the logs
  • applied the suggested tweaks and fixes to the README.md file

Let me know how the new version looks to you.

<body>
<h4>IndexedDB files handling demos:</h4>
<ol>
<li><a href="#" id="reading-saving-files-example">Read a local directory tree and Save its content into IndexedDB</a></li>

Choose a reason for hiding this comment

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

Save -> save

</details>

<details id="step-04">
<summary>2. Retrieve persisted files</summary>

Choose a reason for hiding this comment

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

  1. -> 4.

@wbamberg
Copy link

I think this looks really great! It's much more intuitive (for me at least). I spotted a couple of typos, but otherwise r+ from me.

the logs are now in a column on the right

I almost asked for this, so I'm very glad you did it anyway :).

@rpl
Copy link
Member Author

rpl commented Jan 11, 2017

@mi-g This pull request contains the updated version of the indexeddb-files example, would you mind to take a last look at it as a technical review from an Addon Developer point of view?
Thanks in advance for your help!

@mi-g
Copy link

mi-g commented Jan 12, 2017

@rpl thanks. reply copied from bug#1323414:

Indeed, opening the DB as persistent allowed me to create a 20GB file, so i presume using this mode clears the quota issue.

In the case of VDH like in probably most add-ons, the file download/manipulation is performed in the background and not in a content window. I tried opening the DB as persistent in the background side of the add-on and it did not work (no confirmation UI appeared). Once that dialog opened and has been accepted, the background can open the DB.

For this reason and because showing this dialog message from the add-on would be a terrible UX choice leading users to confusion, a permission should be added to the manifest so users would be notified of the add-on requesting unlimited storage only once at install time.

So, at this time the issues are:

- there is no way (that i could test) to bring a generated virtual file to the real filesystem (downloading a file-to-blob URL fails)
- the manifest should implement a permission to use the indexeddb without quota restrictions
- fixing the previous items would very certainly allow VDH to work, but from a usability point-of-view, controlling the file writing through the download manager API would be much better

@wbamberg wbamberg mentioned this pull request Jan 21, 2017
@andymckay
Copy link

I do like the way this is going, but its rather complicated and I think we should reduce the amount of copy and paste in an example. Is there are a library we can use for doing this, or can we abstract out some of the heavy lifting into a library that can be updated independent of this example?

@rpl
Copy link
Member Author

rpl commented Jan 24, 2017

@andymckay I've another example (which I started before this one, but I chose to finalize after this one was completed) which uses BrowserFS to abstract the filesystem access (and I was thinking about trying to use localForage as another simpler abstraction), unfortunately one of the interesting part of this example is the MutableFileHandle and it is not a standard feature, and so I doubt that any of the existent abstraction take it into account (and they will not provide a nicer API to interfact with it).

One reason why I chose to write this version first is that it is "library agnostic" and it provides an simple example of how to use the underlying features directly, nevertheless I think that the other example (which will use the abstractions) is going to be needed as well.

@andymckay
Copy link

@rpl lets close this in favour of the new library you are building out.

@andymckay andymckay closed this Apr 10, 2017
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.

None yet

4 participants