Skip to content

fs: initial experimental promisified API #15485

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.

Sign up for GitHub

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

Closed
jasnell wants to merge 2 commits into from
Closed

fs: initial experimental promisified API #15485

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0b9086
fs: initial experimental promisified API
jasnell Sep 16, 2017
df1d587
[Squash] address some feedback
jasnell Sep 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
[Squash] address some feedback
  • Loading branch information
@jasnell
jasnell committed Sep 20, 2017
commit df1d5878c83673c73aa4b8b819e0e6d88db18c0f
6 changes: 3 additions & 3 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class FSReqWrap: public ReqWrap<uv_fs_t> {
Local<Value> promise =
object()->Get(context, env->oncomplete_string()).ToLocalChecked();
if (promise->IsPromise()) {
if (promise.As<Promise>()->State() != Promise::kPending) return;
CHECK_EQ(promise.As<Promise>()->State(), Promise::kPending);
Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>();
resolver->Resolve(context, arg).FromJust();
Copy link
Copy Markdown
Member

@addaleax addaleax Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to #5691 you’ll want an InternalCallbackScope around this … ideally move the class definition to node_internals.h in the same way it’s done in #15428 to avoid merge conflicts :)

Copy link
Copy Markdown
Member Author

@jasnell jasnell Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, thanks for the reminder :-)

Copy link
Copy Markdown
Member Author

@jasnell jasnell Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, how and where to use InternalCallbackScope is about as clear as mud right now :-( ... any chance of having some docs written up on it? Even if only in the form of some code comments.

Copy link
Copy Markdown
Member

@addaleax addaleax Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, sure. From node.h:

 * `MakeCallback()` is a wrapper around this class as well as
 * `Function::Call()`. Either one of these mechanisms needs to be used for
 * top-level calls into JavaScript (i.e. without any existing JS stack).

do you have any specific suggestions for how to improve that? should resolving promise state be mentioned explicitly?

Copy link
Copy Markdown
Member Author

@jasnell jasnell Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work up some language.

return;
Expand All @@ -109,7 +109,7 @@ class FSReqWrap: public ReqWrap<uv_fs_t> {
Local<Value> argv[2];
argv[0] = Null(env->isolate());
argv[1] = arg;
MakeCallback(env->oncomplete_string(), 2, argv);
MakeCallback(env->oncomplete_string(), arraysize(argv), argv);
}
}

Expand All @@ -118,7 +118,7 @@ class FSReqWrap: public ReqWrap<uv_fs_t> {
Local<Value> promise =
object()->Get(context, env->oncomplete_string()).ToLocalChecked();
if (promise->IsPromise()) {
if (promise.As<Promise>()->State() != Promise::kPending) return;
CHECK_EQ(promise.As<Promise>()->State(), Promise::kPending);
Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>();
resolver->Reject(context, arg).FromJust();
return;
Expand Down