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.

Sign up for GitHub

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

Jump to bottom

Define state consistency within a revision #223

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Define state consistency within a revision #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
772c697
Initial fix of #197 #215 #220 #222 for moxie crate
Dec 14, 2020
a51a0e6
Rollback public API
Dec 16, 2020
b3fe8c3
Rollback `Defer` implementation for `Key`
Dec 17, 2020
167efaa
Add `poll_once` for `Runloop`
Dec 25, 2020
9100c8d
Docs update
Dec 25, 2020
ef032ec
Removed `Deref` implementation for `Key`;
Dec 29, 2020
cfa7bec
Fix `Var`'s commits so that after `Revision`
Dec 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dom/examples/todo/src/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub fn filter_link(to_set: Visibility) -> Li {
mox! {
<li>
<a style="cursor: pointer;"
class={if *visibility == to_set { "selected" } else { "" }}
class={if *visibility.commit_at_root() == to_set { "selected" } else { "" }}
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't need to acquire a lock here to get the visibility for the revision, but I think this is a problem for you because of my past laziness.

At the top of this function is this attribute:

#[illicit::from_env(visibility: &Key<Visibility>)]

I think this needs to become

#[illicit::from_env(visibility: &Commit<Visibility>, set_visibility: &Key<Visibility>)]

Which will require exposing the current commit via https://github.com/anp/moxie/blob/main/dom/examples/todo/src/lib.rs#L53 as well.

onclick={move |_| visibility.set(to_set)}>
{% "{}", to_set }
</a>
Expand Down
3 changes: 2 additions & 1 deletion dom/examples/todo/src/item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ mod tests {
pub async fn single_item() {
let root = document().create_element("div");
crate::App::boot_fn(&[Todo::new("weeeee")], root.clone(), || {
let todo = &illicit::expect::<Key<Vec<Todo>>>()[0];
let todos_key = &illicit::expect::<Key<Vec<Todo>>>();
let todo = &todos_key.commit_at_root()[0];
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly to my other comment, I think we need to pass down a Commit<Vec<Todo>> from the App.

todo_item(todo)
});

Expand Down
15 changes: 8 additions & 7 deletions dom/examples/todo/src/main_section.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ pub fn toggle(default_checked: bool) -> Span {
#[illicit::from_env(todos: &Key<Vec<Todo>>, visibility: &Key<Visibility>)]
pub fn todo_list() -> Ul {
Copy link
Owner

Choose a reason for hiding this comment

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

See my comments in other todomvc files, I think if you add Commit<Visibility> and Commit<Vec<Todo>> to the illicit environment then you can avoid all the commit_at_root calls in this file.

let mut list = ul().class("todo-list");
for todo in todos.iter() {
if visibility.should_show(todo) {
for todo in todos.commit_at_root().iter() {
if visibility.commit_at_root().should_show(todo) {
list = list.child(todo_item(todo));
}
}
Expand All @@ -46,17 +46,18 @@ pub fn todo_list() -> Ul {
#[topo::nested]
#[illicit::from_env(todos: &Key<Vec<Todo>>)]
pub fn main_section() -> Section {
let num_complete = todos.iter().filter(|t| t.completed).count();
let num_complete = todos.commit_at_root().iter().filter(|t| t.completed).count();

let mut section = section().class("main");

if !todos.is_empty() {
section = section.child(toggle(num_complete == todos.len()));
if !todos.commit_at_root().is_empty() {
section = section.child(toggle(num_complete == todos.commit_at_root().len()));
}
section = section.child(todo_list());

if !todos.is_empty() {
section = section.child(filter_footer(num_complete, todos.len() - num_complete));
if !todos.commit_at_root().is_empty() {
section =
section.child(filter_footer(num_complete, todos.commit_at_root().len() - num_complete));
}

section.build()
Expand Down
67 changes: 25 additions & 42 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ use std::{
use topo::CallId;

/// Applied to impl blocks, this macro defines a new "updater" wrapper type that
/// holds a [`crate::Key`] and forwards all receiver-mutating methods. Useful
/// holds a [`Key`] and forwards all receiver-mutating methods. Useful
/// for defining interactions for a stateful component with less boilerplate.
///
/// Requires the name of the updater struct to generate in the arguments to the
Expand Down Expand Up @@ -285,22 +285,21 @@ where
/// let mut rt = RunLoop::new(|| state(|| 0u64));
///
/// let track_wakes = BoolWaker::new();
/// rt.set_state_change_waker(waker(track_wakes.clone()));
///
/// let (first_commit, first_key) = rt.run_once();
/// let (first_commit, first_key) = rt.run_once_with(waker(track_wakes.clone()));
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be a problem for run_once_with to accept an impl std::task::Wake argument and to do this wrapping internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is nightly-only feature and would be an overhead if someone would pass an std::task::Waker.

/// assert_eq!(*first_commit, 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.set(0); // this is a no-op
/// assert_eq!(*first_key, 0, "no updates yet");
/// assert_eq!(*first_key.commit_at_root(), 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.set(1);
/// assert_eq!(*first_key, 0, "update only enqueued, not yet committed");
/// assert_eq!(*first_key.commit_at_root(), 0, "update only enqueued, not yet committed");
/// assert!(track_wakes.is_woken());
///
/// let (second_commit, second_key) = rt.run_once(); // this commits the pending update
/// assert_eq!(*second_key, 1);
/// assert_eq!(*second_key.commit_at_root(), 1);
/// assert_eq!(*second_commit, 1);
/// assert_eq!(*first_commit, 0, "previous value still held by previous pointer");
/// assert!(!track_wakes.is_woken(), "wakes only come from updating state vars");
Expand Down Expand Up @@ -331,22 +330,21 @@ where
/// let mut rt = RunLoop::new(|| cache_state(&epoch.load(Ordering::Relaxed), |e| *e));
///
/// let track_wakes = BoolWaker::new();
/// rt.set_state_change_waker(futures::task::waker(track_wakes.clone()));
///
/// let (first_commit, first_key) = rt.run_once();
/// let (first_commit, first_key) = rt.run_once_with(futures::task::waker(track_wakes.clone()));
/// assert_eq!(*first_commit, 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.set(0); // this is a no-op
/// assert_eq!(*first_key, 0, "no updates yet");
/// assert_eq!(*first_key.commit_at_root(), 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.set(1);
/// assert_eq!(*first_key, 0, "update only enqueued, not yet committed");
/// assert_eq!(*first_key.commit_at_root(), 0, "update only enqueued, not yet committed");
/// assert!(track_wakes.is_woken());
///
/// let (second_commit, second_key) = rt.run_once(); // this commits the pending update
/// assert_eq!(*second_key, 1);
/// assert_eq!(*second_key.commit_at_root(), 1);
/// assert_eq!(*second_commit, 1);
/// assert_eq!(*first_commit, 0, "previous value still held by previous pointer");
/// assert!(!track_wakes.is_woken(), "wakes only come from updating state vars");
Expand All @@ -363,15 +361,15 @@ where
/// assert!(!track_wakes.is_woken());
///
/// third_key.set(2);
/// assert_eq!(*third_key, 2);
/// assert_eq!(*third_key.commit_at_root(), 2);
/// assert!(!track_wakes.is_woken());
///
/// third_key.set(3);
/// assert_eq!(*third_key, 2);
/// assert_eq!(*third_key.commit_at_root(), 2);
/// assert!(track_wakes.is_woken());
///
/// let (fourth_commit, fourth_key) = rt.run_once();
/// assert_eq!(*fourth_key, 3);
/// assert_eq!(*fourth_key.commit_at_root(), 3);
/// assert_eq!(*fourth_commit, 3);
/// assert_eq!(*third_commit, 2);
/// assert!(!track_wakes.is_woken());
Expand Down Expand Up @@ -630,7 +628,7 @@ where
///
/// Reads through a commit are not guaranteed to be the latest value visible to
/// the runtime. Commits should be shared and used within the context of a
/// single [`crate::runtime::Revision`], being re-loaded from the state variable
/// single [`runtime::Revision`], being re-loaded from the state variable
/// each time.
///
/// See [`state`] and [`cache_state`] for examples.
Expand Down Expand Up @@ -681,7 +679,6 @@ where
/// See [`state`] and [`cache_state`] for examples.
pub struct Key<State> {
id: CallId,
commit_at_root: Commit<State>,
var: Arc<Mutex<Var<State>>>,
}

Expand All @@ -691,6 +688,11 @@ impl<State> Key<State> {
self.id
}

/// Returns the `Commit` of the current `Revision`
pub fn commit_at_root(&self) -> Commit<State> {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's call this method current_value -- commit_at_root was meant to describe the commit at the time when the state var was "bound" in the current revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems a bit confusing if you would have curent_value while latest commits being passed to the "key updater".

self.var.lock().current_commit().clone()
}

/// Runs `updater` with a reference to the state variable's latest value,
/// and enqueues a commit to the variable if `updater` returns `Some`.
/// Returns the `Revision` at which the state variable was last rooted
Expand All @@ -717,22 +719,21 @@ impl<State> Key<State> {
/// let mut rt = RunLoop::new(|| state(|| 0u64));
///
/// let track_wakes = BoolWaker::new();
/// rt.set_state_change_waker(waker(track_wakes.clone()));
///
/// let (first_commit, first_key) = rt.run_once();
/// let (first_commit, first_key) = rt.run_once_with(waker(track_wakes.clone()));
/// assert_eq!(*first_commit, 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.update(|_| None); // this is a no-op
/// assert_eq!(*first_key, 0, "no updates yet");
/// assert_eq!(*first_key.commit_at_root(), 0, "no updates yet");
/// assert!(!track_wakes.is_woken(), "no updates yet");
///
/// first_key.update(|prev| Some(prev + 1));
/// assert_eq!(*first_key, 0, "update only enqueued, not yet committed");
/// assert_eq!(*first_key.commit_at_root(), 0, "update only enqueued, not yet committed");
/// assert!(track_wakes.is_woken());
///
/// let (second_commit, second_key) = rt.run_once(); // this commits the pending update
/// assert_eq!(*second_key, 1);
/// assert_eq!(*second_key.commit_at_root(), 1);
/// assert_eq!(*second_commit, 1);
/// assert_eq!(*first_commit, 0, "previous value still held by previous pointer");
/// assert!(!track_wakes.is_woken(), "wakes only come from updating state vars");
Expand All @@ -744,16 +745,6 @@ impl<State> Key<State> {
var.enqueue_commit(new);
}
}

/// Set a new value for the state variable, immediately taking effect.
fn force(&self, new: State) {
self.var.lock().enqueue_commit(new);
}

// TODO(#197) delete this and remove the Deref impl
fn refresh(&mut self) {
self.commit_at_root = runtime::Var::root(self.var.clone()).0;
}
}

impl<State> Key<State>
Expand Down Expand Up @@ -788,15 +779,7 @@ where

impl<State> Clone for Key<State> {
fn clone(&self) -> Self {
Self { id: self.id, commit_at_root: self.commit_at_root.clone(), var: self.var.clone() }
}
}

impl<State> Deref for Key<State> {
type Target = State;

fn deref(&self) -> &Self::Target {
self.commit_at_root.deref()
Self { id: self.id, var: self.var.clone() }
}
}

Expand All @@ -805,7 +788,7 @@ where
State: Debug,
{
fn fmt(&self, f: &mut Formatter) -> FmtResult {
self.commit_at_root.fmt(f)
self.commit_at_root().fmt(f)
}
}

Expand All @@ -814,7 +797,7 @@ where
State: Display,
{
fn fmt(&self, f: &mut Formatter) -> FmtResult {
self.commit_at_root.fmt(f)
self.commit_at_root().fmt(f)
}
}

Expand Down
Loading