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

Allow skipping a callback when reforming #2864

Merged
merged 4 commits into from Sep 14, 2022
Merged

Allow skipping a callback when reforming #2864

merged 4 commits into from Sep 14, 2022

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Sep 8, 2022

Description

This adds a method to the callback, similar to Rust's filter_map, which allows to reform a callback, but also suppress the emit in case the reform function returns None.

Checklist

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

@github-actions
Copy link

github-actions bot commented Sep 8, 2022 •
edited

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 170.233 170.239 +0.006 +0.003%
communication_child_to_parent 90.466 90.467 +0.001 +0.001%
communication_grandchild_with_grandparent 105.418 105.421 +0.003 +0.003%
communication_grandparent_to_grandchild 101.320 101.319 -0.001 -0.001%
communication_parent_to_child 87.601 87.599 -0.002 -0.002%
contexts 107.849 107.847 -0.002 -0.002%
counter 85.501 85.503 +0.002 +0.002%
counter_functional 86.031 86.030 -0.001 -0.001%
dyn_create_destroy_apps 88.372 88.370 -0.002 -0.002%
file_upload 100.081 100.084 +0.003 +0.003%
function_memory_game 163.779 163.783 +0.004 +0.002%
function_router 348.458 348.471 +0.013 +0.004%
function_todomvc 158.626 158.628 +0.002 +0.001%
futures 221.975 221.988 +0.014 +0.006%
game_of_life 105.739 105.740 +0.001 +0.001%
immutable 181.605 181.604 -0.002 -0.001%
inner_html 82.368 82.369 +0.001 +0.001%
js_callback 111.385 111.383 -0.002 -0.002%
keyed_list 195.571 195.568 -0.003 -0.001%
mount_point 85.122 85.121 -0.001 -0.001%
nested_list 112.596 112.597 +0.001 +0.001%
node_refs 92.851 92.851 0 0.000%
password_strength 1548.171 1548.170 -0.001 -0.000%
portals 96.287 96.284 -0.003 -0.003%
router 318.126 318.120 -0.006 -0.002%
simple_ssr 152.170 152.179 +0.009 +0.006%
ssr_router 394.003 394.010 +0.007 +0.002%
suspense 109.106 109.111 +0.005 +0.004%
timer 88.317 88.317 0 0.000%
todomvc 139.742 139.743 +0.001 +0.001%
two_apps 86.116 86.115 -0.001 -0.001%
web_worker_fib 152.222 152.229 +0.008 +0.005%
webgl 84.834 84.834 0 0.000%

✅ None of the examples has changed their size significantly.

@ctron ctron force-pushed the feature/ext_callback_1 branch 2 times, most recently from 4ca6673 to 7c57013 Compare September 8, 2022 15:07
@github-actions
Copy link

github-actions bot commented Sep 8, 2022 •
edited

Visit the preview URL for this PR (updated for commit 041e4bb):

https://yew-rs-api--pr2864-feature-ext-callback-gglmhxo5.web.app

(expires Mon, 19 Sep 2022 11:59:02 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 8, 2022 •
edited

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 373.476 462.379 402.357 24.759
Hello World 10 713.770 830.146 755.129 40.524
Function Router 10 2849.349 3074.120 2929.271 60.656
Concurrent Task 10 1010.482 1015.673 1011.790 1.598

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 366.345 401.541 379.723 9.981
Hello World 10 698.675 743.722 719.947 14.237
Function Router 10 2792.998 2908.396 2841.046 34.790
Concurrent Task 10 1010.460 1011.612 1010.967 0.386

WorldSEnder
WorldSEnder requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

One suggestion to the documentation, feel free to suggest an even better one.

The test cases seem fine, and the method has a clear purpose to me.

packages/yew/src/callback.rs Outdated Show resolved Hide resolved
@WorldSEnder WorldSEnder added feature-request A feature request A-yew Area: The main yew crate labels Sep 8, 2022
WorldSEnder
WorldSEnder reviewed Sep 8, 2022
View reviewed changes
packages/yew/src/callback.rs Outdated
/// Creates a new callback from another callback and a function
/// That when emitted will call that function and will emit the original callback if it is
/// not `None`.
pub fn filter_reform<F, T>(&self, func: F) -> Callback<T>
Copy link
Member

Choose a reason for hiding this comment

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

One thing I almost forgot: Would it make sense to have this return Callback<T, Option<OUT>> instead - and generalize for any Callback<IN, OUT>?

EDIT: If returning Option<OUT> is not desirable, perhaps requiring OUT: Default and keeping the same OUT as its output makes most sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, that sounds like an interesting idea. I will try to amend the PR with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good. I amended the PR.

hamza1311
hamza1311 reviewed Sep 9, 2022
View reviewed changes
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.

Looks good. Thanks for also improving callback test coverage

Seems like failed test are related to tokio-rs/tokio#4973. Can you update the Cargo.toml to pin tokio to a working version? That should make a CI green

@futursolo
Copy link
Member

CI should now be fixed by #2866 and #2867. Could you please rebase from / merge the latest master branch?

ctron and others added 3 commits September 12, 2022 12:51
@ctron
Allow skipping a callback when reforming
fb6a666 
This adds a method to the callback, similar to Rust's filter_map, which
allows to reform a callback, but also suppress the emit in case
the reform function returns `None`.
@ctron @WorldSEnder
Update packages/yew/src/callback.rs
75fc583 
Co-authored-by: WorldSEnder <WorldSEnder@users.noreply.github.com>
@ctron
Implement filter_reform for all Callback output types
291beaf 
As suggested in the PR, this implements filter_reform for all output
types of callbacks by mapping to `Option<OUT>`.
@ctron
Fix clippy error
041e4bb 
Although I believe that the code is more readable/understandable with
an explicit map, I am not sure adding a clippy exception just for that
is justified. So I applied the clippy suggestion.
futursolo
futursolo approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@futursolo futursolo 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 PR. Looks good.

hamza1311
hamza1311 approved these changes Sep 14, 2022
View reviewed changes
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.

Looks good. This method has a clear use case and is a welcome addition.

Thanks for the PR!

@WorldSEnder WorldSEnder merged commit beb0157 into yewstack:master Sep 14, 2022
@ctron ctron deleted the feature/ext_callback_1 branch September 15, 2022 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WorldSEnder WorldSEnder WorldSEnder approved these changes

@futursolo futursolo futursolo approved these changes

@hamza1311 hamza1311 hamza1311 approved these changes

Assignees
No one assigned
Labels
A-yew Area: The main yew crate feature-request A feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ctron @futursolo @WorldSEnder @hamza1311