Add readonly/writeonly options for volumes

[elmpp]

Adds options object to .use() which allows marking volumes as readonly/writeonly. If a "write" method is attempted on a readonly volume it is effectively a noop and vice-versa.

Notes:

• removes dependency on fs-monkey

Context:

• 425

[G-Rath]

Woah this is super cool!

I've just added prettier to this project, so if you could rebase this PR & write prettier that would be great (easiest way is probably to install prettier yourself, run it and git commit --amend, then rebase - that should give you little-to-no rebase conflicts).

After that, I've requested a few changes :)

[G-Rath]

After playing around with an refactored implementation locally, this can definitely be made a lot simpler by using readable & writable.

Here's a slice of the end result to help guide you:

  use(fs: IFS, options: VolOptions): this {
    this.fss.push(this.createMethods(fs, options));
    return this;
  }

  /**
   * At the time of the [[use]] call, we create our sync, async and promise methods
   * for performance reasons
   *
   * @param fs
   * @param options
   */
  private createMethods(fs: IFS, { readable = true, writable = true }: VolOptions = {}): IFS {

This implementation will mean you can revert the majority of changes in other functions; there are some possible optimisations that can follow on from this, but they can come later in another PR :)

[elmpp]

After playing around with an refactored implementation locally, this can definitely be made a lot simpler by using readable & writable.

Here's a slice of the end result to help guide you:

  use(fs: IFS, options: VolOptions): this {
    this.fss.push(this.createMethods(fs, options));
    return this;
  }

  /**
   * At the time of the [[use]] call, we create our sync, async and promise methods
   * for performance reasons
   *
   * @param fs
   * @param options
   */
  private createMethods(fs: IFS, { readable = true, writable = true }: VolOptions = {}): IFS {

This implementation will mean you can revert the majority of changes in other functions; there are some possible optimisations that can follow on from this, but they can come later in another PR :)

Only problem with defaulting to true for both readonly/writeonly is that we can't ensure consumers don't select nonsensical options of both true?

I guess we could change the options to readable/writeable but i took the gist of this PR to give consumers a kind of "guarantee" that volumes would be unchanged.

[G-Rath]

I'm a bit confused on what you mean; but if I understand you correctly you're referring to the fact that saying "readonly" and "writeonly" is a bit more explicit, which I agree with but this is where it ends you.

The alternative I think would be taking an enum/union value: that way you'd not be able to say readonly & writeonly; i.e

ufs.use(fs, UnionFS.ReadOnly);
ufs.use(fs, UnionFS.WriteOnly);

The issue then is I'm not sure what the best way to represent "neither" is, other than just Neither? While that would be the default value, it's still technically got to have a name and can be passed.

Overall I think an object is probably better because it'll allow us to add additional options in the future if needed.

Regardless:

i took the gist of this PR to give consumers a kind of "guarantee" that volumes would be unchanged.

You still have that guarantee, since the current/default (aka the current implementation on master for #use) is "the volume will be written to & read from", so having readable & writable options simply lay on top of that i.e "the volume will be written to & read from, but is not readable" (then math lets you simplify that to "the volume will only be written to")

[elmpp]

I'm a bit confused on what you mean; but if I understand you correctly you're referring to the fact that saying "readonly" and "writeonly" is a bit more explicit, which I agree with but this is where it ends you.

The alternative I think would be taking an enum/union value: that way you'd not be able to say readonly & writeonly; i.e

ufs.use(fs, UnionFS.ReadOnly);
ufs.use(fs, UnionFS.WriteOnly);

The issue then is I'm not sure what the best way to represent "neither" is, other than just Neither? While that would be the default value, it's still technically got to have a name and can be passed.

Regardless:

i took the gist of this PR to give consumers a kind of "guarantee" that volumes would be unchanged.

You still have that guarantee, since the current/default (aka the current implementation on master for #use) is "the volume will be written to & read from", so having readable & writable options simply lay on top of that i.e "the volume will be written to & read from, but is not readable" (then math lets you simplify that to "the volume will only be written to")

Ah right! You mean dispensing with the options object and having a discriminating enum instead? Right, this is much clearer now - i couldn't marry the concept of readonly & writeonly with an options object :)

So, with that in mind my thoughts are probably to still opt for options as an object? It's kind of canonical to specify as an object and surely we want to keep the door open for more options?

We can try and overload the use() method to prevent both being supplied as true without that explicit check i added? Not entirely sure TS will enforce it though once transpiled for JS consumers...

[G-Rath]

Ah right! You mean dispensing with the options object and having a discriminating enum instead? Right, this is much clearer now - i couldn't marry the concept of readonly & writeonly with an options object :)

Yes, but I was saying that more as it was another possible route.

What I'm saying is change the options interface to be this:

interface FSOptions {
  writable?: boolean;
  readable?: boolean;
}

This way, the value of each property is actually independent of the other: even if they're both false all that will mean is that you'll have a volume that can't be used in any capacity which is fine with us.

This greatly simplified the implementation as well, as you don't need to change any code outside of use.

That's how you get to this:

 use(fs: IFS, options: VolOptions): this {
    this.fss.push(this.createMethods(fs, options));
    return this;
  }

  /**
   * At the time of the [[use]] call, we create our sync, async and promise methods
   * for performance reasons
   *
   * @param fs
   * @param options
   */
  private createMethods(fs: IFS, { readable = true, writable = true }: VolOptions = {}): IFS {
    /* ... */

    return {
      ...fs,
      /* ...reducers */
      promises: { 
         ...fs.promises,
         /* ...reducers */          
      }
    }
  }

So, with that in mind my thoughts are probably to still opt for options as an object? It's kind of canonical to specify as an object and surely we want to keep the door open for more options?

Exactly my thoughts as well :)

[elmpp]

Have implemented the switch from writeonly -> writeable. Took me a while to latch on yesterday! @G-Rath

[G-Rath]

Awesome, looking good! Now the last major simplification to apply is to refactor createMethods to return IFS instead of FSMethodStack - that'll remove the need for most of the other changes in the file :)

[elmpp]

@G-Rath hopefully that does it

[G-Rath]

You've almost there, but you can simplify it further so that you don't have to change the type of the fss field :)

[elmpp]

it's referenced here?

[G-Rath]

Yeah; what I'm saying is that you don't need that.

I'd recommend reverting all of your changes in that file aside from use & createMethods - you should find that the implementation should just work with little to no changes :)

[elmpp]

Yeah; what I'm saying is that you don't need that.

The watch method is a "special" method and not in our explicit lists so we need to know right there whether that FS can be watched..

[G-Rath]

When I tried it last the tests were all passing - that could just mean we've not properly covering that area of code, but I'll have a think about it tomorrow morning (it's 1am here).

[G-Rath]

we need to know right there whether that FS can be watched..

I don't think we do, for the same reason we don't need to anywhere else: if an fs method throws for any reason, we swallow that error & move on to the next fs - and if there are no more fs' left we throw.

Would you be open to humoring me? If you make a commit reverting the changes, we can see in CI what tests - if any - fail, and why.

If all the tests pass, but you can successfully cause an error then we can add a new test to cover that, and then drop the reverting commit.

[elmpp]

I don't think we do, for the same reason we don't need to anywhere else: if an fs method throws for any reason, we swallow that error & move on to the next fs - and if there are no more fs' left we throw.

The "special" methods aren't subject to the same error handling as the "listed" methods and need to be explicitly skipped.

The original concept called for a "readonly/writeonly" api which i implemented. With the api change to a "readable/writeable" paradigm I've dutifully reimplemented it all.

I think improvements could definitely be made to error handling as a whole but probably better as another issue. Can you humour me please here? :)

E2A i think keeping the options around in fss would also be useful down the road for more complex scenarios (i have in mind possibly copying between volumes)

@G-Rath

[G-Rath]

The original concept called for a "readonly/writeonly" api which i implemented. With the api change to a "readable/writeable" paradigm I've dutifully reimplemented it all.

The original concept was the ability to make an fs readable & writable separately, in order for adding fs' that can only be read from, or only be written to.

My initial proposal was meant as a starting point for investigation on how such ability could be implemented. Once I had a look at the code properly, it was immediately clear that the current implementation would be simpler, requiring less changes & being easier to maintain.

This is also why I was, and continue to be, happy to implement the changes I'm requesting myself, because when you made the PR it was for your implementation rather than taking ownership & responsibility for resolving the underlying issue/request.

Can you humour me please here? :)

Unfortunately it's not that simple, because "humouring you" here means merging & releasing a new version with this implementation.

My humouring request was because I'm currently busy with a few other projects, so while I'm happy to spend the time exploring this implementation & seeing if what I think could be possible is possible, I can't promise how soon I'll have the time to do that.

Hence, I was asking since you know the code better if you could spare the time reverting the majority of the code (in a manner that doesn't discard the implementation so it's easy to revert back) so that we could see what actually happens in the CI output.

If you don't want to, that's completely understandable and fine by me 🙂

I think keeping the options around in fss would also be useful down the road for more complex scenarios

I do agree with this; however, I feel it should only be implemented when it's needed, not because it might be useful, to reduce the risk of bugs or behaviour changes.

As it happens, I don't think that the solution to #429 requires any extra information - I'd expect it to be just a matter of internally catching for that method, and then doing a read + write for each volume.

---

The original bottom line here was that I have an implementation of this that passes all tests, which means either this can be simplified, or we don't have coverage on a bug.

However, I only just realised that your implementation is not throwing errors when all the volumes are not read or writable when calling a relative operation, which I don't think is correct.

If I call "read" on a volume that cannot be read from, an error should be thrown, the same as if a file didn't exist, as it's a permissions failure.

If the current tests were correct, then yes you're right, you wouldn't be able to simplify this further.

Are you happy to continue working with me on this PR, or would you prefer that I finish it off?

[elmpp]

Can i continue with my quarantine now or has it ended? :)

[G-Rath]

Can i continue with my quarantine now or has it ended? :)

I don't know what you mean by this.

My bandwidth for reviewing this PR is nearing saturation, so I might not be able to re-review for a bit, but I'll check this out locally when I have the time to see if it's still possible to eliminate the [IFS, VolOptions][].

[elmpp]

Can i continue with my quarantine now or has it ended? :)

I don't know what you mean by this.

My bandwidth for reviewing this PR is nearing saturation, so I might not be able to re-review for a bit, but I'll check this out locally when I have the time to see if it's still possible to eliminate the [IFS, VolOptions][].

Small topical joke as we've been going so long is all.

Can we please push on a wee bit more - I think we're into code-styleee stuff now. Tests look good and api decided etc

[G-Rath]

Had some time to re-review this - it's looking good.

All that's left to do is revert the fss property back to being of type IFS[] (and the related changes), and I think this will be good to go.

For that change, you'll need to change createErroringFn to support calling the callback for async functions:

const createErroringFn = (state: 'readable' | 'writable', asyncMethod: boolean) => (...args: any[]) => {
  const error = new Error(`Filesystem is not ${state}`);
  if(asyncMethod) {
    let cb = args[args.length - 1];
    if (typeof cb === 'function') {
      return cb(error);
    }
  }

  throw error;
};

[G-Rath]

I've got a bit of confusion over the tests that would be awesome to get cleared up.

Also if you can be bothered, refactoring your async tests to use a Promise instead of done would be great!

i.e

test('some test', () => {
  return new Promise(done => {
    expect(false).toBe(true);
    done();
  });
});

[elmpp]

Had some time to re-review this - it's looking good.

All that's left to do is revert the fss property back to being of type IFS[] (and the related changes), and I think this will be good to go.

For that change, you'll need to change createErroringFn to support calling the callback for async functions:

const createErroringFn = (state: 'readable' | 'writable', asyncMethod: boolean) => (...args: any[]) => {
  const error = new Error(`Filesystem is not ${state}`);
  if(asyncMethod) {
    let cb = args[args.length - 1];
    if (typeof cb === 'function') {
      return cb(error);
    }
  }

  throw error;
};

Updated tests, demo etc

The options must be kept around to support the "magic methods". If there are plans to refactor these methods then that would be an ideal time for this surely?

[G-Rath]

The options must be kept around to support the "magic methods".

Not according to the tests.

If there are plans to refactor these methods then that would be an ideal time for this surely?

What is the "that" & "this" you're referring to?

Plans change, as do implementations.

It's generally best to let refactors stand on their own merits, rather than bake them into feature implementations unless they're actually required or clearly an improvement.

One of the reasons for this is that a refactor that makes it easier to implement one particular feature might actually make it harder to implement another, but in the scope of the feature make total sense.

[airhorns]

Is this something you folks are still interested in landing? Would be really great to get in!

[G-Rath]

I'm going to close this due to age - I still think the feature is a very useful one for unionfs to have, so if someone wants to pick this back up I'll try and reserve some time for reviewing.

While it sounded like the implementation in this PR was almost good to go in the end, it had a lot of back and forward + it's been a while for me since I looked over this, so I think if someone does want to pick this up it would be best to do so using a new branch rather than reuse this one (though the implementation itself might be reusable).