Remove asynchronous constructors in favor of factory methods

[jvilk]

Problem

It's awkward to write code like this:

new BrowserFS.FileSystem.IndexedDB(function(e, fs) {
  // Now you can use fs!
});

This is weird, because the constructor also returns fs:

var idbfs = new BrowserFS.FileSystem.IndexedDB(function(e, fs) {
  // idbfs === fs
});

A BrowserFS user could potentially incorrectly grab the fs object and use it before initialization completes.

Some file systems in BrowserFS use a different approach, and have a post-constructor initialization function:

var mirrored = new BrowserFS.FileSystem.AsyncMirror(inMemory, idbfs);
mirrored.initialize(function (e) {
  BrowserFS.initialized(mirrored);
});

To the user of BrowserFS, all of this is awkward, tedious, frustrating, and inconsistent; I apologize for not addressing the problem sooner!

Proposed Change

1. Introduce factory methods in next minor version (e.g., 1.4.0).
2. Deprecate asynchronous constructors and initialization functions in next minor version.
3. Remove asynchronous constructors and initialization functions in next major version (e.g., 2.0.0).

The change would look something like this:

BrowserFS.FileSystem.IndexedDB.Create(function(e, fs) {
  // Now you can use `fs`
});

These factory functions would also be easy to promisify if you are using something like bluebird:

var createIDBFS = Promise.promisify(BrowserFS.FileSystem.IndexedDB.Create);
createIDBFS().then(function(fs) {
  console.log("Success");
}).catch(function(e) {
  console.error(e);
});

[jvilk]

If anyone wants to take a stab at making this change for a single filesystem, let me know!

[disarticulate]

What's the most frustrating is having to search for an hour just to get useful code samples like this. A minor step would be to document these better.

[jvilk]

@disarticulate does the new api documentation help at all? Each FileSystem should have a little snippet of code at the top.

If there are any documentation gaps, feel free to file issues.

[disarticulate]

I don't see any code snippets when i look at the api documents. It's all typescript definitions and...nothing.

It's well documented no doubt, but it doesn't show me how to start using it, unfortunately. I'm just trying to bootstrap a persistence layer and can't get any of these working. If you want someone to rewrite the things, you might have to demonstrate just how to get them working in the first place.

I don't work with Typescript so whatever typings are tell you, they go over my head.

Here's a skeleton that I'm trying to do

export const persistState = {
  getItem (key) => {

  },
  setItem (key, state) => {

  },
  removeItem (key) => {

  },
  clear () => {

  }
}

I can clearly see there's an interface to a keyvalue store, but I can't find it digging through nodejs methods or anything. It looks like you've got all the complex things documented, but my brain isn't accepting how to enter.

[disarticulate]

here's my attempt at promisfying the above:

export const fsState = (t) => { 
  return new Promise((resolve,reject) => {
    new bfs.FileSystem[t]((e, fs) => {
      resolve(fs)
      reject(e)
    })
  })
}

[disarticulate]

This is what I ended up with, which is a plugin for Vuejs persisted state:

import { initialize, FileSystem } from 'browserfs'
import { Buffer } from 'buffer'
console.log(FileSystem)

export const initializeDb = () => {
  return new Promise((resolve, reject) => {
    var memfs = new FileSystem.InMemory()
    initialize(memfs)
    var indbfs = new FileSystem.IndexedDB((e) => {
      resolve({
        memfs: memfs,
        indbfs: indbfs
      })
      reject(e)
    }, 'state')
  })
}

export const persistStateIndexDb = (db) => {
  return {
    getItem: (key) => {
      db.indbfs.store.beginTransaction('readonly').get(key, (e, stateBuffer) => {
        var state = Buffer(stateBuffer).toString('utf8')
        return state
      })
    },
    setItem: (key, state) => {
      var stateBuffer = Buffer.from(state, 'utf8')
      db.indbfs.store.beginTransaction('readwrite').put(key, stateBuffer, true,
        (e, commited) => {
          db.memfs.store[key] = state
          return state
        }
      )
    },
    removeItem: (key) => {
      db.indbfs.store.beginTransaction('readwrite').del(key,
      (e) => {
        return null
      })
    },
    clear: () => {
      /* what */
    }
  }
}

export const persistStateMemory = (db) => {
  return {
    getItem: (key) => {
      console.log('get', key, db.memfs.store[key])
      return db.memfs.store[key]
    },
    setItem: (key, state) => {
      console.log('set', key, state, db.memfs.store[key])
      db.memfs.store[key] = state
      return state
    },
    removeItem: (key) => {
      delete db.memfs.store[key]
    },
    clear: () => {
      /* what */
    }
  }
}

the db being taken is what's getting initialized. You're right, it's difficult to understand why we're initializing the file systems in two different manners.

[jvilk]

@disarticulate Whoa, you're just directly using the key/value stores that the file systems are based on?

BrowserFS's use case is to emulate a file system abstraction. Setting it up takes two steps:

1. Create (and possibly initialize) a file system instance.
   • If you need multiple types of backends, you can mount them at different directory paths within a MountableFileSystem instance. Think of it like mounting disk partitions in an OS.
2. Pass it to BrowserFS.initialize().

Now, you can grab a reference to the fs module with BrowserFS.BFSRequire('fs'), and use it just like Node's fs module. No special per-filesystem-backend interfaces required. The only confusing per-filesystem-backend logic is how to create a new instance of the file system, which is something I'd like to standardize.

What you are doing is breaking that file system abstraction. You're reaching in to two filesystems and directly operating on their key/value stores. The data you are storing this way will not be available through the file system at all, since the filesystem also maintains other metadata alongside file data (including, but not limited to, directory listings).

[jvilk]

Resolved in 1.4.0. See the README and new documentation for more details. BrowserFS.configure makes it significantly easier to configure file systems, and the Create methods are significantly more consistent than the old method of configuring file systems.

The old constructors now have a deprecation notice, and will be removed in 2.x.

[billiegoose]

I like these changes! The new configure command is much more declarative. In case it helps others, I'm posting the warning messages I got (hello future Googlers!), and my code before and after migrating to the new API. Since the author cleverly included a link to this thread in the deprecation messages themselves, I figure this is the place to share!

Here is my previous code. It was a mess, full of temp variables, callbacks, and constructors. (Even inside a callback inside a constructor.)

export const fsReady = new Promise(function(resolve, reject) {
  new BrowserFS.FileSystem.IndexedDB((err, idbfs) => {
    let ajaxFS = new BrowserFS.FileSystem.XmlHttpRequest();
    let overlayFS = new BrowserFS.FileSystem.OverlayFS(idbfs, ajaxFS);
    overlayFS.initialize(() => {
      let mfs = new BrowserFS.FileSystem.MountableFileSystem();
      mfs.mount('/', overlayFS);
      mfs.mount('/orig', ajaxFS);
      // Initialize it as the root file system.
      BrowserFS.initialize(mfs);
      resolve()
    })
  })
})

it still worked. But I started seeing these warnings in the console:

browserfs:1721 [XmlHttpRequest] Direct file system constructor usage is deprecated for this file system, and will be removed in the next major version. Please use the 'XmlHttpRequest.Create({"index":"index.json","baseUrl":""}, callback)' method instead. See https://github.com/jvilk/BrowserFS/issues/176 for more details.
browserfs:12146 [Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
browserfs:1721 [XmlHttpRequest] Direct file system constructor usage is deprecated for this file system, and will be removed in the next major version. Please use the 'XmlHttpRequest.Create({"index":"index.json","baseUrl":""}, callback)' method instead. See https://github.com/jvilk/BrowserFS/issues/176 for more details.
browserfs:1721 [OverlayFS] Direct file system constructor usage is deprecated for this file system, and will be removed in the next major version. Please use the 'OverlayFS.Create({"readable":"readable file system","writable":"writable file system"}, callback)' method instead. See https://github.com/jvilk/BrowserFS/issues/176 for more details.
browserfs:11344 [OverlayFS] OverlayFS.initialize() is deprecated and will be removed in the next major release. Please use 'OverlayFS.Create({readable: readable file system instance, writable: writable file system instance}, cb)' to create and initialize OverlayFS instances.

Here's my new code. After reading this thread, and the newer docs, I decided to try the BrowserFS.configure( route rather than the nested .Create( route initially suggested by the deprecation messages. It's more LOC but those lines are much less dense and way less error-prone. It's just a single statement with a JSON argument!

export const fsReady = new Promise(function(resolve, reject) {
  BrowserFS.configure({
    fs: "MountableFileSystem",
    options: {
      "/": {
        fs: "OverlayFS",
        options: {
          writable: {
            fs: "IndexedDB",
            options: {}
          },
          readable: {
            fs: "XmlHttpRequest",
            options: {}
          }
        }
      },
      "/orig": {
        fs: "XmlHttpRequest",
        options: {}
      }
    }
  }, (err) => err ? reject(err) : resolve())
})

I experienced two minor "gotchas".

1. Don't misspell writable. I had "writeable" and got this error:

   browserfs:10443 Uncaught (in promise) TypeError: Cannot read property 'isReadOnly' of undefined
   at new UnlockedOverlayFS (browserfs:10443)
   at new OverlayFS (browserfs:11315)
   at Function.Create (browserfs:11327)

2. The "options" turned out not to be optional for XmlHttpRequest. Is this a bug? I initially got this error until I added options: {}:

   Uncaught (in promise) TypeError: Cannot read property 'index' of undefined
   at Function.Create (browserfs:12673)
   at finish (browserfs:15303)
   at getFileSystem (browserfs:15336)
   at eval (browserfs:15315)
   at Array.forEach (<anonymous>)
   at getFileSystem (browserfs:15311)
   at eval (browserfs:15315)
   at Array.forEach (<anonymous>)
   at getFileSystem (browserfs:15311)
   at Object.configure (browserfs:15272)

Now I only get the warning about Synchronous XMLHttpRequest, which is what I'd expect:

browserfs:12146 [Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.

[jvilk]

@wmhilton glad to hear it's working as intended. :)

Most backends require at least one option. XmlHttpRequest needs an index property containing either:

• The URL to your index.json
• The actual index object

The index is generated using the make_xhrfs_index script. Without the index, XHRFS would not know where directories are, and what their contents are.

[jvilk]

Maybe at some point I'll work on better error messages when options are missing; a FS could contain a metadata object describing the properties it expects to find.

[jvilk]

I just realized that the old behavior had a default value for index. Whoops!

[billiegoose]

I kind of assumed the default location for index.json was "index.json". Do I need to start explicitly setting that URL?