-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Pass hook dependencies as the first function argument #2861
Conversation
There are multiple hooks that are defined in the order of IMO, I don't think there is significant benefit of swapping the order of the argument and this is a pretty fundamental change. Could you please provide an example of how this improves things over the previous design? |
There is a reason for swapping the order, I think this was discussed somewhere on discord, too. 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. |
Sounds reasonable. I am in favour of this change if all hooks are updated to have a uniformed signature. |
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 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
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 |
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 |
@cecton This tool is AMAZING! Here is what you would need to run to refactor:
I also agree with the api change, feels less javascripty and more rusty <3 |
e3d0220
to
d8bd715
Compare
I updated the PR as it seems OP eventually abandoned it. |
website/sidebars/docs.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we forgot to add migration guides to the Next sidebar. I added missing links as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here now you have a second approval haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we adjust the order of other hooks with dependencies as well so they are consistent (such as: use_memo, use_callback...)?
} | ||
|
||
// Never equals, so this will be called every render | ||
use_effect_base(|_| f(), NeverEq, |_, _| true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the NeverEq type is not needed any more, as the should render function is saying true all the time.
(I don't quite remember the original implementation of this PR, but I think this is modified to always use PartialEq so a special type is needed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the original change: https://github.com/voidpumpkin/yew/compare/db15696..86b6cb4
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.) |
6060273
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) |
I checked the refactoring commands on a big codebase. Did not run into any issues. |
Description
Fixes #0000
Checklist