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

Define streams common operations in Infrastracture section. #54

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

yutakahirano
Copy link
Owner

This is a refactoring effort. Addressing #52.


A __ReadableStream__ represents a stream of data, as specified in https://streams.spec.whatwg.org. In this section, we define common operations for ReadableStream used in this spec.

There are two kinds of signals, __pull__ and __cancel__. They can be signalled to a ReadableStream.
Copy link

Choose a reason for hiding this comment

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

Where is "signalled" defined?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nowhere. I wanted a kind of event dispatching without DOMEvent complexity (Event interface, bubbling, multiple listeners, ...). I thought "signal" was simple enough like "terminate" used in the fetch spec.
Do you know a defined word suited with such use-case?

Copy link

Choose a reason for hiding this comment

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

Maybe it's okay for now. I don't really have a better alternative. Other than maybe callback algorithm or some such... (Ideally "terminate" would be defined from first principles too.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be rephrased to be a bit simpler. I would remove all of this about signals, then define "To construct a ReadableStream with given strategy, pull action, and cancel action, run these steps."

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds nice. As we don't need pull signal in "need more data" definition any more, we can replace signals with actions. Done.

@domenic
Copy link
Contributor

domenic commented Aug 27, 2015

So, this over all looks pretty reasonable, but it doesn't actually address the biggest part of #52, which is IDL vs. JS values. Each defined algorithm probably needs a step at the beginning converting inputs into JS values and a step at the end converting outputs into IDL values. (Or, a blanket "in these steps we conflate IDL and JS values" disclaimer.)

@yutakahirano
Copy link
Owner Author

@domenic

In #50 and #52 some solutions were proposed and we didn't choose one. I would like to collect JS-stream related definitions in one place so that we can deal with the problem easily in the future (i.e. I won't close #52 after this PR is landed, but add "not-included-in-the-first-release" label to it and start creating merge PRs).

I can add "we don't distinguish JS-stream and IDL-stream as of now" note in this PR.

@yutakahirano
Copy link
Owner Author

@domenic, are you fine with this change?

1. Let _promise_ be a new promise.
1. Let _bytes_ be an empty byte sequence.
1. Let _read_ be the result of calling [ReadFromReadableStreamReader(_reader_)].
1. When _read_ is resolved with an object whose `done` property is __false__ and whose `value` property is a Uint8Array, append the `value` property to _bytes_ and run the above step again.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/resolved/fulfilled here and in the next two steps.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

@domenic
Copy link
Contributor

domenic commented Sep 2, 2015

Yes, LGTM with some minor comments. Good stuff.

This is a refactoring effort. Addressing #52.
@yutakahirano yutakahirano force-pushed the infrastructure-refactoring branch from 0408752 to f4354e5 Compare September 2, 2015 06:17
@yutakahirano
Copy link
Owner Author

Thank you!

yutakahirano added a commit that referenced this pull request Sep 2, 2015
Define streams common operations in Infrastracture section.
@yutakahirano yutakahirano merged commit 8040025 into master Sep 2, 2015
@yutakahirano yutakahirano deleted the infrastructure-refactoring branch October 6, 2015 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants