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

Implement sleep and interval for Yew Platform #2784

Merged
merged 13 commits into from Jul 31, 2022

Conversation

futursolo
Copy link
Member

Description

This pull request implements yew::platform::time which provides sleep() and interval() support.

Checklist

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

@futursolo futursolo added the A-yew Area: The main yew crate label Jul 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
@github-actions
Copy link

github-actions bot commented Jul 19, 2022

Visit the preview URL for this PR (updated for commit 4f1973c):

https://yew-rs-api--pr2784-sleep-interval-bsns9r0r.web.app

(expires Thu, 28 Jul 2022 13:33:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
@github-actions
Copy link

github-actions bot commented Jul 19, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 172.759 172.781 +0.022 +0.013%
contexts 109.618 109.609 -0.009 -0.008%
counter 86.588 86.588 0 0.000%
counter_functional 87.243 87.240 -0.003 -0.003%
dyn_create_destroy_apps 89.730 89.711 -0.020 -0.022%
file_upload 102.815 102.814 -0.001 -0.001%
function_memory_game 166.928 166.947 +0.020 +0.012%
function_router 351.926 351.866 -0.060 -0.017%
function_todomvc 161.652 161.657 +0.005 +0.003%
futures 225.427 225.431 +0.004 +0.002%
game_of_life 107.172 107.196 +0.024 +0.023%
immutable 208.737 208.733 -0.004 -0.002%
inner_html 83.705 83.702 -0.003 -0.004%
js_callback 112.807 112.808 +0.001 +0.001%
keyed_list 195.270 195.272 +0.003 +0.002%
mount_point 86.232 86.233 +0.001 +0.001%
nested_list 115.676 115.677 +0.001 +0.001%
node_refs 93.528 93.524 -0.004 -0.004%
password_strength 1546.206 1546.210 +0.004 +0.000%
portals 97.321 97.319 -0.002 -0.002%
router 321.103 321.083 -0.020 -0.006%
simple_ssr 154.321 154.320 -0.001 -0.001%
ssr_router 397.940 397.920 -0.021 -0.005%
suspense 110.422 110.471 +0.049 +0.044%
timer 89.292 89.320 +0.028 +0.032%
todomvc 142.683 142.683 0 0.000%
two_apps 87.253 87.252 -0.001 -0.001%
web_worker_fib 153.670 153.675 +0.005 +0.003%
webgl 87.604 87.603 -0.002 -0.002%

✅ None of the examples has changed their size significantly.

github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 19, 2022
@futursolo futursolo mentioned this pull request Jul 20, 2022
2 tasks
# Conflicts:
#	packages/yew/Cargo.toml
#	packages/yew/tests/use_reducer.rs
github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 21, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 21, 2022
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.

Sorry, it took so long to get to this. Looks good!

Comment on lines +14 to +66
pub struct Sleep {
inner: Option<Timeout>,
dur_left: Option<u128>,
timeout_registered: Rc<Cell<bool>>,
}

impl Future for Sleep {
type Output = ();

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
static I32_MAX_U128: u128 = 2_147_483_647;
static I32_MAX_U32: u32 = 2_147_483_647;

// If polling before the registered timeout is reached, return Pending.
if self.timeout_registered.get() {
return Poll::Pending;
}

// set_timeout can only accept maximum of i32, so we wrap around if it gets longer.
let next_timeout = match self.dur_left.map(|m| (m, u32::try_from(m))) {
Some((m_u128, Err(_))) => {
self.dur_left = Some(m_u128 - I32_MAX_U128);
I32_MAX_U32
}
Some((m_u128, _)) if m_u128 > I32_MAX_U128 => {
self.dur_left = Some(m_u128 - I32_MAX_U128);
I32_MAX_U32
}
Some((_, Ok(m_u32))) => {
self.dur_left = None;
m_u32
}
None => return Poll::Ready(()),
};

let waker = cx.waker().clone();
self.timeout_registered.set(true);
let timeout_registered = self.timeout_registered.clone();

self.inner = Some(Timeout::new(next_timeout, move || {
timeout_registered.set(false);
waker.wake();
}));

Poll::Pending
}
}

Sleep {
inner: None,
dur_left: Some(dur.as_millis()),
timeout_registered: Cell::new(false).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 implement it here? What's wrong with the gloo implementation? If there's a bug or improvement to be made, I think we should fix it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

See: rustwasm/gloo#121

The difficulty of patching it with gloo is due to its callback-first API.

Patching gloo_timers requires recursively calling setTimeout, which is difficult to implement due to ownership rules.
It gets worse with Interval, as we have to implement it with a series of patched Timeout than using setInterval.
At that time, it's simply easier to implement Interval with Stream and make the API future-first.
However, this is a bigger change than it is needed to patch around this behaviour.

I patched it here because we have a future-first API and this have very little impact on code size.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. I'm not sure who actually is using a timer that's over 24 days long but I'm fine with merging this change.

Patching it in Gloo would be needless (and not small) breaking change

@futursolo futursolo merged commit de613fa into yewstack:master Jul 31, 2022
@futursolo futursolo deleted the sleep-interval branch December 15, 2022 10:12
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants