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

Make fn update() re-render the component by default #2786

Merged

Conversation

cecton
Copy link
Member

@cecton cecton commented Jul 20, 2022

Description

Make Component's method fn update() re-render the component by default. See #1961 (comment)

Putting this to true is very handy because it allows a basic component that only needs to refresh with a callback to refresh without having to define the fn update() method.

Related to #1961

Checklist

  • I have reviewed my own code
  • I have added tests (I should probably add a test to make sure someone don't revert it back inadvertently/without discussion)

@github-actions
Copy link

github-actions bot commented Jul 20, 2022

Visit the preview URL for this PR (updated for commit 0f50ca9):

https://yew-rs-api--pr2786-cecton-update-compon-cfkeqm2z.web.app

(expires Wed, 27 Jul 2022 14:30:48 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@cecton cecton marked this pull request as ready for review July 20, 2022 14:11
github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
@github-actions
Copy link

github-actions bot commented Jul 20, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 173.431 173.427 -0.004 -0.002%
contexts 110.138 110.179 +0.041 +0.037%
counter 87.038 87.037 -0.001 -0.001%
counter_functional 87.701 87.705 +0.004 +0.004%
dyn_create_destroy_apps 90.149 90.146 -0.003 -0.003%
file_upload 103.500 103.504 +0.004 +0.004%
function_memory_game 168.630 168.637 +0.007 +0.004%
function_router 353.755 353.823 +0.068 +0.019%
function_todomvc 162.418 162.416 -0.002 -0.001%
futures 226.104 226.107 +0.004 +0.002%
game_of_life 107.697 107.697 0 0.000%
immutable 210.053 210.045 -0.008 -0.004%
inner_html 84.057 84.100 +0.043 +0.051%
js_callback 113.381 113.386 +0.005 +0.004%
keyed_list 195.875 195.924 +0.049 +0.025%
mount_point 86.680 86.681 +0.001 +0.001%
nested_list 116.255 116.289 +0.034 +0.029%
node_refs 93.980 93.983 +0.003 +0.003%
password_strength 1547.374 1547.376 +0.002 +0.000%
portals 97.867 97.870 +0.003 +0.003%
router 321.681 321.885 +0.204 +0.063%
simple_ssr 154.938 154.940 +0.003 +0.002%
ssr_router 400.144 400.173 +0.029 +0.007%
suspense 111.044 111.037 -0.007 -0.006%
timer 89.729 89.731 +0.002 +0.002%
todomvc 143.397 143.399 +0.002 +0.001%
two_apps 87.691 87.693 +0.002 +0.002%
web_worker_fib 154.685 154.694 +0.010 +0.006%
webgl 87.948 87.948 0 0.000%

✅ None of the examples has changed their size significantly.

github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
@cecton
Copy link
Member Author

cecton commented Jul 20, 2022

This PR is ready.

@hamza1311
Copy link
Member

To answer as to why I made update return false: It was like this in the original PR #1646

See: https://github.com/yewstack/yew/pull/1646/files#diff-f25ab267a13ad40d72e4e71be976e17a39cb8eb5afb632e047a00dc6a3e42f0bR19-R24
In yew/src/component/mod.rs (since GitHub doesn't seem to load the file by default)

@cecton
Copy link
Member Author

cecton commented Jul 20, 2022

Thanks! I will investigate further

@cecton
Copy link
Member Author

cecton commented Jul 25, 2022

@hamza1311 so I asked Justin by email (didn't want to bother him with notifications) and he said:

Yeah I think it's just to put some value there

So I guess it's safe to merge 🙌

@WorldSEnder
Copy link
Member

WorldSEnder commented Jul 25, 2022

Yeah I think it's just to put some value there

So I guess it's safe to merge 🙌

I suppose, a component that doesn't want to receive messages should use an empty message type, i.e. ! instead of () so I think changing the default has some value (albeit being slightly weird, since by default receiving a message doesn't change the state).

@cecton
Copy link
Member Author

cecton commented Jul 25, 2022

I never thought about that but you are correct. Sometimes, in Yew 0.18, I make an empty enum (which is equivalent to !) to show that there is no message at all to handle. Most of the time I'm too lazy so I use ().

fn update(msg: Msg) -> bool {
    match msg {}
}

On the other hand, with component v2, we don't need to write a type for a component when we don't use messages so there is no need to be explicit about that.

However, it's particularly handy to have the default message on () because you can send () through the link easily and you can refresh the component like that.

albeit being slightly weird, since by default receiving a message doesn't change the state

True... Minor detail though... imo it's not like you send messages by mistake.

@hamza1311
Copy link
Member

However, it's particularly handy to have the default message on () because you can send () through the link easily and you can refresh the component like that.

Just wondering, what can a refresh without updating any state do? You can't modify any component state from view because it takes &self

The only place I can see it being useful are components which use interior mutability, but at that point, I imagine the component the component will be complex enough to have a message type

I was thinking maybe we could default the message type to !. It has to be done with a nightly flag though

@cecton
Copy link
Member Author

cecton commented Jul 28, 2022

Just wondering, what can a refresh without updating any state do? You can't modify any component state from view because it takes &self

If your component's rendering depends on external variables, then refreshing is very useful. For example, when using a global state for your application you would want to refresh components independently.

I was thinking maybe we could default the message type to !.

It makes sense alright but it's unnecessary as nobody really cares what is the type of the message when the message is not used. I mean you can have ! or (), it won't have any impact whatsoever for the developer who use Yew.

Making the component refresh is a bit of a niche trick but it doesn't harm anyone: anybody who define their own message will override this function anyway.

It has to be done with a nightly flag though

Would that force yew to nightly? I would prefer that we keep Yew in stable unless there is a very important feature. In this case, using ! instead of () doesn't bring enough value imo.

@hamza1311
Copy link
Member

hamza1311 commented Jul 31, 2022

I've should've been been more clear. I agree with the change.

Would that force yew to nightly? I would prefer that we keep Yew in stable unless there is a very important feature. In this case, using ! instead of () doesn't bring enough value imo.

No, we have a feature flag that users can enable if they're on nightly.

nightly = ["yew-macro/nightly"]

It really should be a compiler flag though: #2754

@hamza1311 hamza1311 added the A-yew Area: The main yew crate label Jul 31, 2022
@hamza1311 hamza1311 added this to the v0.20 milestone Jul 31, 2022
@cecton
Copy link
Member Author

cecton commented Aug 1, 2022

Thanks! I'll wait a bit to get a second approval to be safe

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.

There is some potential source of confusion from re-renders of components if these "spurious" update messages ever get used in a library facility (think checks such as done React Strict Mode) but they would most likely be opt-in by default and I don't want to design around future possibilities anyway. Completely fine right now, for poking your own components.

@cecton cecton merged commit c802167 into yewstack:master Aug 1, 2022
@cecton cecton deleted the cecton-update-component-by-default-when-updated branch August 1, 2022 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants