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

xilem_web: Add MemoizedStream view #757

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

flosse
Copy link
Contributor

@flosse flosse commented Nov 23, 2024

The MemoizedStream view is is basically a MemoizedAwait view that accepts a stream and invokes the callback for each stream item received.
Since the implementation is almost the same, the two views are just wrappers for the private MemoizedInner struct.
If the approach is okay, I can also add an example.

@flosse flosse added the web label Nov 23, 2024
@flosse flosse requested a review from Philipp-M November 23, 2024 04:16
@flosse flosse marked this pull request as ready for review November 23, 2024 04:17
xilem_web/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

I added a few comments, no real blockers though, thanks!

(Ideally we would also have an example showing a use-case for this, but I don't want to block on that...)

xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
xilem_web/src/concurrent/memoized_await.rs Outdated Show resolved Hide resolved
@flosse
Copy link
Contributor Author

flosse commented Nov 25, 2024

(Ideally we would also have an example showing a use-case for this, but I don't want to block on that...)

Of course, that will come.
Unfortunately, this week looks very full for me, but apart from myself, probably nobody is waiting at the moment 😉

log::debug!("Run app logic");

let search_stream = memoized_stream(
(state.search_term.clone(), state.db.clone()),
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 a bit messy, because the first argument (state.search_term) is what should be monitored,
the second (state.db) is only there to make it available for stream generation (in create_search_stream).

Copy link
Contributor Author

@flosse flosse Nov 26, 2024

Choose a reason for hiding this comment

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

@Philipp-M
I have now changed the implementation so that the app state is injected again before the future is initialized.
This has two advantages:

  1. Only the data that is to be monitored for changes needs to implement PartialEq and Clone
  2. The point in time at which the future is created is now easier to grasp.

The API now looks like this:

memoized_await(
    50, // data to watch
    |state, watched_data| {
        state.async_call_started = true; // before it was debounced
        my_aync_call(state.non_watched_data, watched_data)
    },
    |state, result| { / * ... */ }
)

The implementation might be not the best, but at least in my manual tests it worked 😉

@flosse flosse force-pushed the xilem_web-memoized-stream branch from 4aed1f3 to 261aa42 Compare November 26, 2024 12:30
@flosse flosse requested a review from Philipp-M November 26, 2024 12:46
Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

Thanks for the example!

I hope it's not too much to ask for and I think we can also use this and iterate in later PRs.
Though I would've thought of something that's more of a real-world use-case (and possibly use html instead of console log, as this more obviously shows what the example is about).
Maybe with inspiration from something like this, with e.g. something like this: https://docs.rs/wasm-streams/latest/wasm_streams/readable/struct.ReadableStream.html#method.into_stream

F: Future<Output = FOut> + 'static,
InitFuture: Fn(&Data) -> F + 'static,
InitFuture: Fn(&mut State, &Data) -> F + 'static,
Copy link
Contributor

Choose a reason for hiding this comment

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

I've thought about adding this, but have hoped that the memoized data is enough (as this does make this even more imperative).
This is relevant. I.e. we should find a good (intuitive but non-magic/foot-gunny) way to handle this imperative stuff more generally.

That said I think we can add that, I've done similar in #452 (though possibly a reason why I haven't merged that yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not a final solution, but i changed it now at least to a immutable reference:

-InitFuture: Fn(&mut State, &Data) -> F + 'static,
+InitFuture: Fn(&State, &Data) -> F + 'static,

schedule_update_fn: None,
schedule_update_timeout_handle: None,
update: false,
thunk: Rc::new(thunk),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think this probably is fine, but I'd like to avoid having an Rc<MessageThunk> here if possible. I.e. that all the provided routing information and async execution is in either View::build/rebuild (IIRC that's currently the case for all other async views).

I've used an Option<impl Future> in #452, maybe more like this?

@flosse flosse marked this pull request as draft December 9, 2024 17:23
@flosse flosse force-pushed the xilem_web-memoized-stream branch from aa831b8 to 96c0ac6 Compare December 22, 2024 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants