jvilk / BrowserFS

BrowserFS is an in-browser filesystem that emulates the Node JS filesystem API and supports storing and retrieving files from various backends.
Other
3.06k stars 215 forks source link

Let BrowserFS.configure() return a Promise instead of requiring a callback #195

Closed billiegoose closed 6 years ago

billiegoose commented 6 years ago

Currently, BrowserFS.configure() is a little awkward if you are not using BrowserFS.install(window). You end up with something like:

BrowserFS.configure({
  fs: "LocalStorage"
}, (e) => {
  if (e) throw e;
  const fs = BrowserFS.BFSRequire('fs');
  /*DO STUFF WITH FS*/
});

Then it gets more awkward if you have multiple modules acting on the file system, because I don't think you want to call BrowserFS.configure once in each module. So what I've been doing in my project is wrapping the file system as a const Promise like this:

// filesystem.js
export const Files = new Promise(function(resolve, reject) {
  BrowserFS.configure({
    fs: "LocalStorage"
  }, (err) => err ? reject(err) : resolve(BrowserFS.BFSRequire('fs')))
})

so I can use it like this:

// app.js
import Files from './filesystem'
Files.then(fs => /*DO STUFF WITH FS*/)

This PR would make the callback to BrowserFS.configure optional. If it detects there's no callback, instead of returning void it returns a Promise<FileSystem>. This simplifies the weird boilerplate in my example 'filesystem.js' above so it looks like this:

// filesystem.js (PR edition)
export const Files = BrowserFS.configure({
  fs: "LocalStorage"
})

If you like the idea I can try to add a test case and update the documentation.

billiegoose commented 6 years ago

I just realized that I introduced the only occurrence of a Promise in the codebase, aside from one in the DropboxFS test factory. There might be a reason for that. Also, that would be awkward to only have one API call that used Promises. I may have been over-thinking with this PR.

jvilk commented 6 years ago

I've avoided using Promises for the following reasons:

So, I think I will close this and let users of the library choose whether or not they want to "promisify" the portions of BrowserFS they use.

billiegoose commented 6 years ago

Sounds good to me! I was just getting carried away.