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

Adding support for promises. Closes #379, #382 #380

Merged
merged 7 commits into from Aug 27, 2018

Conversation

@dsych
Copy link
Contributor

commented Aug 22, 2018

  1. Attaching promises object on FileSystem similar to how node implements it.
    • Adding deprecation warning to exists method, since it is obsolete in node, see.
    • Promise is always rejected, when promise-based exists is called.
  2. Attaching promises object on Shell that returns promise-based version of all method accept for pwd.

Simple test code to see how it works.

Note: I am using provider for node.

const fs = new FileSystem({provider: new nodejs({name: guid(), keyPrefix: guid()})}, async (err, fs) => {
  const Shell = fs.Shell;
  const sh = new Shell().promises;
  fs = fs.promises;
  const fd = await fs.open('/myfile', 'w+');
  await fs.close(fd);
  const stats = await fs.stat('/myfile');
  console.log(stats);

  await sh.touch('/file1');
  console.log(await sh.ls('/'));
});

#379 #382

@dsych dsych referenced this pull request Aug 22, 2018
@humphd

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

We just landed my eslint fixes on develop, can you (or did you?) rebase? Also, what's up with all the spacing changes?

In general, I think this looks good, but I'd like to review it more closely after you deal with anything related to the above.

@humphd

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

This also seems to have broken a ton of tests somehow, see https://travis-ci.org/filerjs/filer/builds/419287914?utm_source=github_status&utm_medium=notification. Maybe rebasing will help.

@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

It seems to be failing here. Not sure why, since I am doing the same thing in the example above...

@dsych dsych force-pushed the dsych:promisify branch from 07c58af to 8d79984 Aug 23, 2018
@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

The culprit was parcel not being able to bundle node's util library. So instead I opted for an equivalent npm package.

@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

Another thing, one can argue that we don't need to write unit tests for any of the promise based api here, since they are re-using the same code that is already tested.

@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2018

@humphd I added a couple of things, do you mind taking a look?

@modeswitch

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Another thing, one can argue that we don't need to write unit tests for any of the promise based api here, since they are re-using the same code that is already tested.

That's an assumption that happens to be true right now. Unit tests ensure that your assumption continues to be true going forward. It's precisely why we do want unit tests.

Copy link
Contributor

left a comment

Left a few questions and suggestions. Also, we need to update README.md to reflect the presence of the new .promises API on the fs and shell.

I'm happy for unit tests to come in follow-up PRs vs. needing it all here. But we will indeed want them for all methods. I'll set my students loose on the problem in a few weeks.

var err = new Errors.EFILESYSTEMERROR('filesystem unavailable, operation canceled');
return callback.call(fs, err);
}
[

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Are these tabs? Can we not do all these unnecessary whitespace changes so the history isn't affected for things unrelated to your fix?

This comment has been minimized.

Copy link
@dsych

dsych Aug 26, 2018

Author Contributor

This is done because I want to bind this reference to promises object in the same loop that regular functions are attached to the prototype. For that, I need to put the loop inside the constructor, this is where extra spaces are coming from. Previously the loop was in the global scope, now it is inside the function.

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Understood, thanks for clarifying.

callback(error);
}
};
FileSystem.prototype.promises[methodName] = promisify(FileSystem.prototype[methodName].bind(fs));

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Newline above this please.

@@ -35,6 +35,7 @@
},
"dependencies": {
"base64-arraybuffer": "^0.1.5",
"es6-promisify": "^6.0.0",

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Why this vs. the builtin util.promisify?

This comment has been minimized.

Copy link
@dsych

dsych Aug 26, 2018

Author Contributor

As I mentioned previously, for some reason parcel is not able to bundle utils module from node.

The culprit was parcel not being able to bundle node's util library. So instead I opted for an equivalent npm package.

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Understood, thanks for educating me. Has anyone filed a bug with Parcel on this? Seems like they should support it, no?

This comment has been minimized.

Copy link
@modeswitch

modeswitch Aug 27, 2018

Member

Unlike webpack, I think parcel does not provide polyfills for Node built-ins.

};
FileSystem.prototype.promises[methodName] = promisify(FileSystem.prototype[methodName].bind(fs));
});
FileSystem.prototype.promises['exists'] = function() {

This comment has been minimized.

Copy link
@humphd

humphd Aug 26, 2018

Contributor

Newline above this, and also please add a comment block above this explaining why we special case this method.

Copy link
Contributor Author

left a comment

@humphd I appreciate your feedback, please check my answers.

var err = new Errors.EFILESYSTEMERROR('filesystem unavailable, operation canceled');
return callback.call(fs, err);
}
[

This comment has been minimized.

Copy link
@dsych

dsych Aug 26, 2018

Author Contributor

This is done because I want to bind this reference to promises object in the same loop that regular functions are attached to the prototype. For that, I need to put the loop inside the constructor, this is where extra spaces are coming from. Previously the loop was in the global scope, now it is inside the function.

@@ -35,6 +35,7 @@
},
"dependencies": {
"base64-arraybuffer": "^0.1.5",
"es6-promisify": "^6.0.0",

This comment has been minimized.

Copy link
@dsych

dsych Aug 26, 2018

Author Contributor

As I mentioned previously, for some reason parcel is not able to bundle utils module from node.

The culprit was parcel not being able to bundle node's util library. So instead I opted for an equivalent npm package.

@dsych dsych force-pushed the dsych:promisify branch from 118da37 to d4a9af6 Aug 27, 2018
Copy link
Contributor

left a comment

Nice, thank you. I left some feedback on a few small things.

README.md Outdated
@@ -110,6 +110,21 @@ Like node.js, callbacks for methods that accept them are optional but suggested
you omit the callback, errors will be thrown as exceptions). The first callback parameter is
reserved for passing errors. It will be `null` if no errors occurred and should always be checked.
#### Support for Promises is here!

This comment has been minimized.

Copy link
@humphd

humphd Aug 27, 2018

Contributor

Change this to "Support for Promises" please.

This comment has been minimized.

Copy link
@dsych

dsych Aug 27, 2018

Author Contributor

Done

README.md Outdated
@@ -110,6 +110,21 @@ Like node.js, callbacks for methods that accept them are optional but suggested
you omit the callback, errors will be thrown as exceptions). The first callback parameter is
reserved for passing errors. It will be `null` if no errors occurred and should always be checked.
#### Support for Promises is here!
Promise based API mimics the way Node [implements](https://nodejs.org/api/fs.html#fs_fs_promises_api) them. Both `Shell` and `FileSystem` now have `promises` object attached alongside regular callback style methods. Method names are identical to their callback counterparts with the difference that instead of receiving third argument as a callback, they return a Promise that is resolved or rejected based on the success of method execution.

This comment has been minimized.

Copy link
@humphd

humphd Aug 27, 2018

Contributor

A few things to fix in this paragraph:

  • "Promise based API" should be "The Promise based API"
  • "now have promises object" should be "now have a promises object"
  • "alongside regular callback style methods" should be "alongside the regular callback style methods"
  • "instead of receiving third argument as a callback" should be "instead of taking a final callback argument"

Otherwise, looks great.

This comment has been minimized.

Copy link
@dsych

dsych Aug 27, 2018

Author Contributor

Done

const fs = new Filer.FileSystem().promises;
fs.open('/myfile', 'w+')
.then(fd => fs.close(fd))
.then(() => fs.stat('/myfile'))

This comment has been minimized.

Copy link
@humphd

humphd Aug 27, 2018

Contributor

Let's do two things here:

  1. Let's also show the non-promises way of doing this, so people can compare the difference. That is, let's have two code blocks and call out the difference between the two so people can clearly understand what you're doing here with respect to the original API.

  2. Let's consider using fstat above, and closing the file descriptor toward the end, so the example is less contrived. So the order would be open, fstat, close.

This comment has been minimized.

Copy link
@dsych

dsych Aug 27, 2018

Author Contributor

I used example from the Overview section to keep it consistent. I could just link my example to the Overview or re-write it completely as you suggested using fstat.

I am inclined to go with the first option of mere linking, since it will reduce clutter of an already code-rich readme. Let me know what you think.

This comment has been minimized.

Copy link
@dsych

dsych Aug 27, 2018

Author Contributor

We can still indicate the differences between styles while linking the examples.

@humphd
humphd approved these changes Aug 27, 2018
Copy link
Contributor

left a comment

One more nit to fix, and I think this is good on my end.

@modeswitch can do a review next.

README.md Outdated
@@ -110,6 +110,21 @@ Like node.js, callbacks for methods that accept them are optional but suggested
you omit the callback, errors will be thrown as exceptions). The first callback parameter is
reserved for passing errors. It will be `null` if no errors occurred and should always be checked.
#### Support for Promises!

This comment has been minimized.

Copy link
@humphd

humphd Aug 27, 2018

Contributor

Remove the '!'.

@humphd humphd requested a review from modeswitch Aug 27, 2018
if(error) {
callback(error);
}
// explicitly throw for 'exists' method, since it is deprecated and the equivalent promise-based implementation does not exist.

This comment has been minimized.

Copy link
@modeswitch

modeswitch Aug 27, 2018

Member

It's safe to omit this for the promise API. If someone tries to use it they will get an error anyway and it's less code to maintain.

@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

@modeswitch @humphd ok I think it is ready for a final review.

@dsych dsych changed the title Adding support for promises. Adding support for promises. #379, #382 Aug 27, 2018
@dsych dsych changed the title Adding support for promises. #379, #382 Adding support for promises. Closes #379, #382 Aug 27, 2018
@modeswitch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@dsych @humphd What did you two decide about tests? I'd like to avoid introducing new code without tests (for a number of reasons that I'm happy to enumerate upon request).

@dsych

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2018

I'm happy for unit tests to come in follow-up PRs vs. needing it all here. But we will indeed want them for all methods. I'll set my students loose on the problem in a few weeks.
#380 (review)

@modeswitch

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

@dsych OK, sounds good.

r+

@humphd humphd merged commit 353290a into filerjs:develop Aug 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@humphd

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

Great job @dsych, thank you for pushing this into the tree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.