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

Implemented UnderlyingSource and added a trivial ReadableStream constructor #254

Merged
merged 6 commits into from
Jan 24, 2024

Conversation

xavierm02
Copy link
Contributor

@xavierm02 xavierm02 commented Dec 5, 2023

Description

Implemented UnderlyingSource, and added a trivial ReadableStream constructor that only transforms its first input into an UnderlyingSource.

A few other things were added to make this possible / easier:

  • A wrapper type JsFn<T, I, O> for JsFunction that allows to be more explicit about the JS function we expect to have, and to automatically handle the TryFromJs and IntoJs conversion for inputs and outputs.
  • A Todo placeholder type for types from the spec that have not been defined yet.
  • Type aliases that allow referring to some types by their WebIDL names.

Manual testing

cargo run --bin jstz -- repl
>> new ReadableStream()
thread 'main' panicked at 'not yet implemented', jstz_api/src/stream/readable/mod.rs:39:9
cargo run --bin jstz -- repl
>> new ReadableStream({type:"bytes"})
thread 'main' panicked at 'not yet implemented', jstz_api/src/stream/readable/mod.rs:39:9
cargo run --bin jstz -- repl
>> new ReadableStream({type:"invalid"})
Uncaught TypeError: invalid is not a valid value for enumeration ReadableStreamType.

One can also add

underlying_source.cancel(Some(boa_engine::js_string!("reason").into()), context);

in the constructor of ReadableStreamClass and then test it with

cargo run --bin jstz -- repl
>> new ReadableStream({cancel:function(reason){console.log(reason);}})
[🪵] reason
thread 'main' panicked at 'not yet implemented', jstz_api/src/stream/readable/mod.rs:42:9
Uncaught TypeError: invalid is not a valid value for enumeration ReadableStreamType.

Checklist

  • Changes follow the existing code style (use make fmt-check to check)
  • Tests for changes have been added
  • Internal documentation has been added (if appropriate)
  • Testing instructions have been added to PR

Copy link
Contributor

@Lesenr1 Lesenr1 left a comment

Choose a reason for hiding this comment

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

👍

crates/jstz_core/src/js_fn.rs Outdated Show resolved Hide resolved
Comment on lines 193 to 206
/// **cancel(reason), of type UnderlyingSourceCancelCallback**
/// A function that is called whenever the consumer cancels the stream, via stream.cancel() or reader.cancel(). It takes as its argument the same value as was passed to those methods by the consumer.
///
/// Readable streams can additionally be canceled under certain conditions during piping; see the definition of the pipeTo() method for more details.
///
// For all streams, this is generally used to release access to the underlying resource; see for example § 10.1 A readable stream with an underlying push source (no backpressure support).
///
/// If the shutdown process is asynchronous, it can return a promise to signal success or failure; the result will be communicated via the return value of the cancel() method that was called. Throwing an exception is treated the same as returning a rejected promise.
///
/// [spec]: https://streams.spec.whatwg.org/#dom-underlyingsource-cancel
///
/// *Even if the cancelation process fails, the stream will still close; it will not be put into an errored state. This is because a failure in the cancelation process doesn’t matter to the consumer’s view of the stream, once they’ve expressed disinterest in it by canceling. The failure is only communicated to the immediate caller of the corresponding method.*
///
/// *This is different from the behavior of the close and abort options of a WritableStream's underlying sink, which upon failure put the corresponding WritableStream into an errored state. Those correspond to specific actions the producer is requesting and, if those actions fail, they indicate something more persistently wrong.*
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe: split long comments into multiple lines ?

Copy link
Contributor

@Lesenr1 Lesenr1 left a comment

Choose a reason for hiding this comment

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

LGTM

@xavierm02 xavierm02 force-pushed the xavierm02@api/stream/underlying-source branch from 49abf91 to b12ebc5 Compare December 6, 2023 15:08
crates/jstz_api/src/stream/readable/underlying_source.rs Outdated Show resolved Hide resolved
crates/jstz_api/src/stream/readable/underlying_source.rs Outdated Show resolved Hide resolved
crates/jstz_core/src/js_fn.rs Outdated Show resolved Hide resolved
crates/jstz_core/src/js_fn.rs Outdated Show resolved Hide resolved
@xavierm02 xavierm02 force-pushed the xavierm02@api/stream/underlying-source branch 2 times, most recently from 2aca812 to eba20f3 Compare January 19, 2024 12:24
@xavierm02 xavierm02 force-pushed the xavierm02@api/stream/underlying-source branch from eba20f3 to 36f2a08 Compare January 19, 2024 15:25
Copy link

Deployed at sr17g48DgXQqTZGdiNhn6d4Ar9qQd12tWnwh

@johnyob johnyob merged commit 71d5256 into main Jan 24, 2024
10 checks passed
@johnyob johnyob deleted the xavierm02@api/stream/underlying-source branch January 24, 2024 16:05
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