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

Agent v2 #2773

Merged
merged 119 commits into from Sep 18, 2023
Merged

Agent v2 #2773

merged 119 commits into from Sep 18, 2023

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Jul 3, 2022

Description

This pull request consists of the following changes:

  • Adapts the Worker v2 format.
  • Agents are now managed via context providers.
  • Adds Oneshot Agent (For each agent a single input and a single output is expected)
  • Adds Reactor Agent (For each agent, multiple inputs and multiple outputs are expected)
  • SSR support for memorised Tasks. (deferred for a later pull request)

This pull request needs the following pull requests:

Questions to solve:

  • In Rust, a running asynchronous coroutine is also commonly named task. This may cause confusion to users. I do not have a better name for this, so I am open to suggestions to an alternative name than task.

Still missing:

  • tests
  • documentation

Checklist

  • I have reviewed my own code
  • I have added tests

@futursolo futursolo marked this pull request as ready for review August 29, 2023 10:57
github-actions[bot]
github-actions bot previously approved these changes Aug 29, 2023
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

We should have a struct components example too

examples/README.md Outdated Show resolved Hide resolved
move |e| link.send_message(Self::Message::WorkerMsg(e))
};
let worker = Worker::bridge(Rc::new(cb));
spawn_local(async move {
Copy link
Member

Choose a reason for hiding this comment

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

I would like there to be a way to avoid calling spawn_local directly. Can we abstract over it somehow? Perhaps using suspense here such that the component renders when the value is available from the worker?

Copy link
Member Author

Choose a reason for hiding this comment

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

A Suspense-capable hook should also be SSR-capable.
Currently, I haven't found a good way to provide the same guarantee of agents in SSR.
In particular, global static is different for each agents spawned and shared for all the forked agents, but that is not true for SSR even if they are spawned into a dedicated thread.
To provide the same level of isolation, we need to spawn child processes in SSR, which is more complicated.

In addition, Suspense should only be used to resolve query (idempotent) futures, not mutations.
We cannot guarantee all oneshot agents don't have side-effects and can be safely executed as many times as the the renderer wants to. So it is needed to provide a mutation-like hook where the output can be executed via a future.

<title>Yew • Web Worker Prime</title>

<link data-trunk rel="rust" href="Cargo.toml" data-bin="app" data-type="main" data-weak-refs />
<link data-trunk rel="rust" href="Cargo.toml" data-bin="worker" data-type="worker" data-weak-refs />
Copy link
Member

Choose a reason for hiding this comment

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

Why not use data-loader-shim here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because .spawn() and .spawn_with_loader()can provide proper tree shaking depending on which function is called. However, I do not think loader={true} will provide proper tree shaking.

I think a potential way to work it to provide it via a trait.
(a.k.a: <WorkerProvider<Codec, ShimLoader> ...> vs <WorkerProvider<Codec, DefaultLoader> ...>)

However, there is no good way of knowing whether tree shaking works unless actually try compiling it after implementing it. So I excluded it from this release.

I also feel it might be OK to require data-shim-loader for all agents given we properly document how the loader shim works.

Comment on lines +12 to +30
#[proc_macro_attribute]
pub fn reactor(attr: TokenStream, item: TokenStream) -> TokenStream {
let item = parse_macro_input!(item as AgentFn<ReactorFn>);
let attr = parse_macro_input!(attr as AgentName);

reactor_impl(attr, item)
.unwrap_or_else(|err| err.to_compile_error())
.into()
}

#[proc_macro_attribute]
pub fn oneshot(attr: TokenStream, item: TokenStream) -> TokenStream {
let item = parse_macro_input!(item as AgentFn<OneshotFn>);
let attr = parse_macro_input!(attr as AgentName);

oneshot_impl(attr, item)
.unwrap_or_else(|err| err.to_compile_error())
.into()
}
Copy link
Member

Choose a reason for hiding this comment

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

Why can't the gloo-worker macros be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because gloo_worker_macros references gloo_worker in generated code. yew-agent-macro needs to reference yew-agent in generated code.

In addition, it is not possible to expose anything other than procedural macros from a macro crate.
A new crate is needed from the gloo side to expose the implementations of procedural macros.

The macro implementation of yew-agent and gloo-worker also differs in diagnostics (agents are named agents instead of workers).

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to expose gloo-worker-macro-impl crate (that can also be compiled to wasm) and then call it from there. It's fine for now. I presume we'd need this crate this yew-agent-macro either way so I'm fine with merging duplicate implementation of it.

packages/yew-agent/src/oneshot/hooks.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/oneshot/provider.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/reactor/hooks.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/reactor/hooks.rs Outdated Show resolved Hide resolved

/// A hook to subscribe to the outputs of a [Reactor] agent.
///
/// All outputs sent to current bridge will be collected into a slice.
Copy link
Member

Choose a reason for hiding this comment

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

Why collect them into a slice instead of providing a stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 2 hooks for reactor and worker agents - use_*_bridge and use_*_subscription.

The use_*_bridge hook uses a callback-like scheme where each output from agents will call the callback function.
The use_*_subscription hook mimics a common useSubscription -style hook from common React GraphQL clients.
For Subscriptions, past messages are collected into a slice. So one can do things like:

let messages_html = messages_data
    .iter()
    .map(|m| html! {
        <div key={m.id}>{m.content}</div>
    })
    .collect::<Vec<_>>();

Another reason to avoid a stream is to avoid a spawn_local.
If a stream is provided, then a future must be spawned to actively poll the stream.

If users do not need things to be collected into a slice, then they can use the _bridge variant with a callback.

Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense. It would be good to have an example in the doc comment.

packages/yew-agent/src/reactor/provider.rs Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Sep 2, 2023
@futursolo
Copy link
Member Author

We should have a struct components example too

I think it is fine with without struct component examples.
Just like the tutorial is switched to all function components, we should continue to promote users to use function components.

Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

I skimmed through the macro code and it looks good to me.

We should have a struct components example too

I think it is fine with without struct component examples. Just like the tutorial is switched to all function components, we should continue to promote users to use function components.

I agree that we should promote users towards function components. we should still absolutely support struct components and provide examples on how to use features with them. I'm fine with merging the PR without it so we're not blocking the release (I would like to get all the 0.21 version s released as soon as possible), we should definitely add those examples later


I'm wondering if there's a way to have an async function that is run inside a worker and the result of it can be .awaited on the main thread?

Comment on lines +12 to +30
#[proc_macro_attribute]
pub fn reactor(attr: TokenStream, item: TokenStream) -> TokenStream {
let item = parse_macro_input!(item as AgentFn<ReactorFn>);
let attr = parse_macro_input!(attr as AgentName);

reactor_impl(attr, item)
.unwrap_or_else(|err| err.to_compile_error())
.into()
}

#[proc_macro_attribute]
pub fn oneshot(attr: TokenStream, item: TokenStream) -> TokenStream {
let item = parse_macro_input!(item as AgentFn<OneshotFn>);
let attr = parse_macro_input!(attr as AgentName);

oneshot_impl(attr, item)
.unwrap_or_else(|err| err.to_compile_error())
.into()
}
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to expose gloo-worker-macro-impl crate (that can also be compiled to wasm) and then call it from there. It's fine for now. I presume we'd need this crate this yew-agent-macro either way so I'm fine with merging duplicate implementation of it.


/// A hook to subscribe to the outputs of a [Reactor] agent.
///
/// All outputs sent to current bridge will be collected into a slice.
Copy link
Member

Choose a reason for hiding this comment

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

I see, that makes sense. It would be good to have an example in the doc comment.

@futursolo
Copy link
Member Author

I'm wondering if there's a way to have an async function that is run inside a worker and the result of it can be .awaited on the main thread?

Isn't that the oneshot worker / agent?

https://github.com/rustwasm/gloo/blob/master/examples/markdown/src/lib.rs#L5

@futursolo futursolo merged commit eec0758 into yewstack:master Sep 18, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate A-yew-agent Area: The yew-agent crate breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants