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

Move from .wait() to .ready #243

Closed
domenic opened this issue Nov 4, 2014 · 7 comments
Closed

Move from .wait() to .ready #243

domenic opened this issue Nov 4, 2014 · 7 comments

Comments

@domenic
Copy link
Member

domenic commented Nov 4, 2014

After the work in #234 lands wait() will no longer be side-effecting. So it should become a property, not function. .ready is a good name.

@yutakahirano
Copy link
Member

"ready" sounds "it's ready to read data when fulfilled" to me, but it's not. I don't have a great idea, but I think .wait() is better than .ready.

@domenic
Copy link
Member Author

domenic commented Nov 10, 2014

The main thing is it needs to be a property not a method.

I think ready works OK? Except for when it transitions from waiting to closed, I guess it is a bit misleading. Other ideas welcome though.

@tyoshino
Copy link
Member

Let me add some more details about what I and @yutakahirano chatted.

Even when .wait is fulfilled, as we already notice, it's possible that the readable stream is "errored" in the fulfillment callback because the underlying source may make the readable stream errored after calling enqueue() before the microtask for the fulfillment callback is executed (*). So, the fulfillment callback should check the .state by itself again to decide what to do on the readable stream. It's distinction between not only "readable" and "closed" but also "errored".

(.closed is ok since all the transitions that trigger fulfill/reject of closedPromise are final.)

It's true that there transition to "readable" or "closed" has happened and we wanted to notify user of that. However, it may still appear misleading considering the possible transition to "errored" if we give names that look indicating non-error state.

  • .readable: not good
  • .wait: looks ok?
  • .ready: does this look implying success? implying readableness?

So, we also questioned if we really should reject .wait for transition to "errored". This behavior is not useless. But, ... due to the fact (*), we still need to write code to handle "errored" state not only in the rejection callback but also in the fulfillment callback. Shouldn't .wait be a fulfill-only promise?

@domenic
Copy link
Member Author

domenic commented Nov 10, 2014

I think you are very right that it should be fulfill-only. "ready" seems like an OK name still in the sense of "no longer waiting" but I'll think about it more. Maybe there is a better adjective.

@tyoshino
Copy link
Member

Both .wait and .ready look fine as far as well documented.

@yutakahirano wdyt?

@yutakahirano
Copy link
Member

"wait" method sounds good. The caller will get the notification of a state transition from waiting.
"wait" property is not good - I don't like a verbal property name.
"ready" property is acceptable, if well documented.

@domenic
Copy link
Member Author

domenic commented Nov 24, 2014

I implemented the rename in a commit that was largely search-and-replace. The issue of making .ready fulfill-only is a bit trickier so I will open a new issue to continue discussion.

domenic added a commit that referenced this issue Nov 26, 2014
Part of #245; see discussions in #243 (comment). Consumers should always either check the state property, or count on the fact that read() will throw when used on an errored stream.
domenic added a commit that referenced this issue Dec 1, 2014
Part of #245; see discussions in #243 (comment). Consumers should always either check the state property, or count on the fact that read() will throw when used on an errored stream.
domenic added a commit that referenced this issue Dec 9, 2014
Part of #245; see discussions in #243 (comment). Consumers should always either check the state property, or count on the fact that read() will throw when used on an errored stream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants