-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Revamp JobQueue
into JobExecutor
and introduce NativeAsyncJob
#4118
Conversation
Test262 conformance changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I'll have to take a look on my computer but this is the first thing I noticed reading the examples.
NativeAsyncJob
NativeAsyncJob
and revamp JobQueue
to `JobExecutor
NativeAsyncJob
and revamp JobQueue
to `JobExecutorJobQueue
into JobExecutor
and introduce NativeAsyncJob
c123722
to
a1bcc77
Compare
/// The [`Future`] job returned by a [`NativeAsyncJob`] operation. | ||
pub type BoxedFuture<'a> = Pin<Box<dyn Future<Output = JsResult<JsValue>> + 'a>>; | ||
|
||
/// An ECMAScript [Job] that can be run asynchronously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any scenarios where these are not ran asynchronously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, an implementor can decide to block on each future if they want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nice work, I think the examples look good and would do well with some discoverability. I've created an issue to improve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments I think could improve but nothing blocking. Good work.
a1bcc77
to
5dd1972
Compare
5dd1972
to
9c35fcd
Compare
This changes the API of
JobQueue
to take a non_exhaustive enumJob
, which should avoid breaking changes in the future. Some other changes:JobQueue
toJobExecutor
.JobPromise
, which is a wrapper aroundNativeJob
but clearly states that the job will push promises forward.NativeAsyncJob
, an equivalent toNativeJob
that isFuture
-compatible.