filerjs / filer

Node-like file system for browsers
BSD 2-Clause "Simplified" License
617 stars 154 forks source link

Using filer with webpack #762

Closed bcheidemann closed 3 years ago

bcheidemann commented 3 years ago

This pull request includes shims for filers fs and path modules that allow them to be easily used as replacements for the node.js equivalents.

The fs shim ensures that methods on the fs module wait for the filesystem to be ready before being executed so that developers don't have to worry about the intricacies of setting up filer and can use fs much as they would in node.

I've also added shims for the file system providers (default, indexeddb and memory) which essentially re-expose existing code but in a place that makes sense to use as an alias in webpack.

Finally, I've written documentation detailing exactly how to set up your webpack config aliases.

Why? At my company we switched to using filer from BrowserFS in our product. As we have an electron version of our web app (or visa versa?) we want to use the same node.js imports (e.g. import fs from 'fs') for all of our calls to filer when we're running in the web. This is easy for us to do using webpack as we have two different configs for the web and electron. This also gives us typescript on the filer methods! =P

If there's any changes you'd like made or if you have any questions I'd be more than happy to oblige! =)

Love what you're doing here, best fs replacement we've found for the web (and we've been through a few) ๐Ÿ‘

bcheidemann commented 3 years ago

Ben, this looks really good. Thanks for taking the time to update the code to work with webpack, and for such an excellent PR. I enjoyed reading about how you've successfully been able to use Filer at your company. It's a passion project for a few of us, so it's helpful to have people let us know that it has been useful.

Can I convince you to add some tests for this? It doesn't have to block this PR, if you're willing to do them later, but I'd like to see us get coverage of this new code.

I'll make you a deal: you write the tests and I'll update all our deps and get this updated on npm.

Hi thanks for reviewing this so promptly! You have yourself a deal! =) I will work on adding tests tomorrow. Truth be told, this is my first time doing anything like this PR so I wanted to hold off on writing proper tests for it before I got some feedback =P

codecov-io commented 3 years ago

Codecov Report

Merging #762 (9c66956) into master (75f2a70) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   86.89%   86.89%           
=======================================
  Files          16       16           
  Lines        1740     1740           
=======================================
  Hits         1512     1512           
  Misses        228      228           

Continue to review full report at Codecov.

Legend - Click here to learn more ฮ” = absolute <relative> (impact), รธ = not affected, ? = missing data Powered by Codecov. Last update 75f2a70...986ad37. Read the comment docs.

bcheidemann commented 3 years ago

@humphd I started writing a test for the fs shim but when running the test I'm getting the error ReferenceError: regeneratorRuntime is not defined. The issue seems to be caused by my use of async/await. I have refactored the fsPromises proxy from:

const fsPromises = new Proxy(fsInstance.promises, {
  get(target, prop) {
    return async (...args) => {
      await fsReady;
      return await target[prop](...args);
    };
  },
});

to:

const fsPromises = new Proxy(fsInstance.promises, {
  get(target, prop) {
    return (...args) => {
      return new Promise((resolve) => {
        fsReady.then(
          () => {
            target[prop](...args).then(
              (value) => {
                resolve(value);
              }
            );
          }
        );
      });
    };
  },
});

This seems to have solved the issue, however the code is a lot less readable now and for that reason I would prefer to keep the original implementation if possible =P

I believe we can solve the problem (without modifying the code) by using babel pre-processing, however I'm by no means an expert in babel or your testing setup so I don't want to go down a rabbit hole without getting your input first!

EDIT: I looked into the issue again this morning. It seems we need to use the regenerator-runtime package. I updated the karma config accordingly and the tests pass. If this is okay with you then I'll commit my changes and update the PR ๐Ÿ‘

humphd commented 3 years ago

I think including the extra weight of the regenerator-runtime in the tests is fine, but I'd like to avoid it in the production builds. Without seeing your code, it sounds like you just need the babel polyfill in the testing environment, is that right? If so, go ahead.

bcheidemann commented 3 years ago

I think including the extra weight of the regenerator-runtime in the tests is fine, but I'd like to avoid it in the production builds. Without seeing your code, it sounds like you just need the babel polyfill in the testing environment, is that right? If so, go ahead.

Yeah I don't think it's necessary in production ๐Ÿ‘

humphd commented 3 years ago

@bcheidemann how is your test coming?

bcheidemann commented 3 years ago

@bcheidemann how is your test coming?

Hi @humphd, apologies for the long delay! I have been quite busy at work and haven't had time to update the PR until now. I've now added tests for all of the shims - let me know if these are up to scratch of if you'd like any changes made. I also added a shim for Buffer and documentation on how to use this with third party libraries that do not import/require it ๐Ÿ‘

All the best, Ben