Add use_effect_with

[arniu]

Description

Fixes #0000

Checklist

• [x] I have reviewed my own code
• [ ] I have added tests

[futursolo]

There are multiple hooks that are defined in the order of (closure, deps). If we were to change the order of use_effect_with_deps, the order of other hooks would need to be changed as well.

IMO, I don't think there is significant benefit of swapping the order of the argument and this is a pretty fundamental change. The existing order matches the order of React hooks and it would be familiar to users coming from React.

Could you please provide an example of how this improves things over the previous design?

[WorldSEnder]

There is a reason for swapping the order, I think this was discussed somewhere on discord, too. Consider that you want to use the same value to compute a dependency and move it by value into the closure. Then, writing

impl SomeLargeT {
    fn id(&self) -> u32; // Only need to use the id as cache key
}
let some_dep: SomeLargeT = todo!();

{
    let id = some_dep.id(); // Have to extract it in advance, some_dep is moved already in the second arg
    use_effect_with_dep(move |_| { todo!(); drop(some_dep); }, id);
}
use_effect_with(some_dep.id(), move |_| { todo!(); drop(some_dep); });

So from my point of view, it makes sense to have the dependencies before the closure. Doing this uniformly for all hooks though would be good to see.

[futursolo]

Sounds reasonable. I am in favour of this change if all hooks are updated to have a uniformed signature.

[github-actions[bot]]

Visit the preview URL for this PR (updated for commit 5a7a035):

https://yew-rs--pr2861-use-effect-hook-d0d3ir42.web.app

_{(expires Sun, 09 Apr 2023 19:52:28 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[github-actions[bot]]

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	371.438	371.800	371.569	0.107
Hello World	10	679.748	682.434	680.693	0.810
Function Router	10	2242.471	2269.385	2254.268	9.903
Concurrent Task	10	1007.171	1008.850	1007.728	0.610
Many Providers	10	1835.836	1980.480	1874.248	43.113

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	371.418	372.186	371.654	0.227
Hello World	10	679.395	685.083	681.960	1.681
Function Router	10	2282.305	2324.680	2308.769	14.055
Concurrent Task	10	1006.776	1008.365	1007.531	0.500
Many Providers	10	1853.999	1951.953	1873.426	28.965

[github-actions[bot]]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	104.199	104.199	0	0.000%
boids	174.132	174.132	0	0.000%
communication_child_to_parent	94.914	94.914	0	0.000%
communication_grandchild_with_grandparent	105.841	105.841	0	0.000%
communication_grandparent_to_grandchild	102.019	102.019	0	0.000%
communication_parent_to_child	92.247	92.247	0	0.000%
contexts	108.443	108.443	0	0.000%
counter	90.289	90.289	0	0.000%
counter_functional	90.624	90.624	0	0.000%
dyn_create_destroy_apps	93.140	93.140	0	0.000%
file_upload	104.142	104.142	0	0.000%
function_memory_game	166.488	166.488	0	0.000%
function_router	333.572	333.438	-0.135	-0.040%
function_todomvc	162.001	162.001	0	0.000%
futures	227.547	227.547	0	0.000%
game_of_life	110.438	110.438	0	0.000%
immutable	186.136	186.136	0	0.000%
inner_html	86.941	86.941	0	0.000%
js_callback	112.552	112.552	0	0.000%
keyed_list	200.873	200.873	0	0.000%
mount_point	90.052	90.052	0	0.000%
nested_list	113.543	113.543	0	0.000%
node_refs	97.101	97.101	0	0.000%
password_strength	1544.560	1544.560	0	0.000%
portals	98.089	98.089	0	0.000%
router	305.024	305.024	0	0.000%
simple_ssr	143.052	143.052	0	0.000%
ssr_router	370.384	370.249	-0.135	-0.036%
suspense	109.661	109.661	0	0.000%
timer	93.164	93.164	0	0.000%
todomvc	144.483	144.483	0	0.000%
two_apps	90.943	90.943	0	0.000%
web_worker_fib	154.892	154.892	0	0.000%
webgl	89.579	89.579	0	0.000%

✅ None of the examples has changed their size significantly.

[ranile]

This change sounds good. I'm fine with it as long as all hooks are updated. A way to automatically update the usages would be great too since it is a pretty big breaking change

[cecton]

It looks like quite a lot of work to update. This one in particular is also harder to fix with regexes.

I wonder if we could make some kind of tool dedicated to Yew that uses rust's parser and we can give it to the users so they can also use it in their code. Here is a project that looks like it will do the job but I didn't manage to make it work: https://crates.io/crates/ast-grep

[voidpumpkin]

@cecton This tool is AMAZING!

Here is what you would need to run to refactor:

sg --pattern 'use_effect_with_deps($T,$$$DEPS)' --rewrite 'use_effect_with($$$DEPS $T)' -l rs -i

I also agree with the api change, feels less javascripty and more rusty <3

[voidpumpkin]

I updated the PR as it seems OP eventually abandoned it. Still, @arniu THANK YOU, a lot, or the initial idea and effort!

[futursolo]

I also agree with the api change, feels less javascripty and more rusty <3

Speaking of being rusty:

Instead of:

use_effect_with(deps, |deps| {});

Wouldn't the following be more rusty?

use_with(deps)
    .effect(|deps| {});

or:

use_effect(|deps| {})
    .with(deps);

or:

hook(|deps| {})
    .with(deps)
    .as_effect();

(I just wrote the above as an idea. I do not think we should make this change in this pull request.)

[ranile]

Can we adjust the order of other hooks with dependencies as well so they are consistent (such as: use_memo, use_callback...)?

I agree here. Also, this PR should provide some way to automate swapping the order of arguments and renaming the hook at call sites, perhaps by AST manipulation (I haven't looked at it closely so it's possible that it already does, in which case, disregard my comment)

[voidpumpkin]

I checked the refactoring commands on a big codebase. Did not run into any issues.