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

Fix #2553 - automatically convert closure to callback for component properties #2554

Merged
merged 4 commits into from Mar 27, 2022

Conversation

finnbear
Copy link
Contributor

@finnbear finnbear commented Mar 27, 2022

Description

I added three functions to facilitate passing closures to component properties of type Callback. As noted in #2553, the documentation implies that this functionality already exists.

Fixes #2553

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@finnbear finnbear changed the title Fix 2553 - automatically convert closure to callback for component properties Fix #2553 - automatically convert closure to callback for component properties Mar 27, 2022
@futursolo futursolo added ergonomics A-yew Area: The main yew crate labels Mar 27, 2022
@github-actions
Copy link

github-actions bot commented Mar 27, 2022

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

https://yew-rs-api--pr2554-2553-support-closure-vz042zvr.web.app

(expires Sun, 03 Apr 2022 18:51:34 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

Size Comparison

examples master (KB) pull request (KB) diff
boids 311.396 311.396 0
contexts 233.374 233.374 0
counter 164.551 164.551 0
dyn_create_destroy_apps 172.900 172.900 0
file_upload 194.975 194.975 0
function_memory_game 351.672 351.672 0
function_router 22.254 22.254 0
function_todomvc 326.895 326.895 0
futures 362.343 362.343 0
game_of_life 207.215 207.215 0
inner_html 156.412 156.412 0
js_callback 171.899 171.899 0
keyed_list 329.066 329.066 0
mount_point 163.723 163.723 0
nested_list 225.381 225.381 0
node_refs 170.800 170.800 0
password_strength 1851.781 1851.781 0
portals 184.307 184.307 0
router 588.999 588.999 0
suspense 222.804 222.804 0
timer 170.535 170.535 0
todomvc 270.630 270.630 0
two_apps 166.010 166.010 0
webgl 170.885 170.885 0

@WorldSEnder
Copy link
Member

Thanks, also for adding test cases. The build failure is probably due to mismatching error reporting from macro outputs, where an additional "the following implementations were found" line is shown. You can check this by heeding the advice from the failing build:

note: If the actual output is the correct output you can bless it by rerunning
your test with the environment variable TRYBUILD=overwrite

on your local machine, and see what changes would be committed to the respective, expected .stderr files. Then add those changes to the PR and we'll proceed.

@finnbear
Copy link
Contributor Author

finnbear commented Mar 27, 2022

I've tried the following but, despite producing some failures, none of them produce any diff in the repository. I searched for mismatch in the output, but didn't find it.

$ TRYBUILD=overwrite cargo make tests
$ TRYBUILD=overwrite cargo test --all-targets --workspace --exclude website-test
$ TRYBUILD=overwrite cargo make pr-flow

$ rustup override set 1.56 && TRYBUILD=overwrite cargo make tests

Also, I'm confused why the unit tests seemingly pass on stable but not 1.56.0. Looking closer, it seems like fewer tests are run.

Edit: I looked for the mismatches and could confirm at least one of them is as expected:

    = help: the following implementations were found:
             <Option<&'static str> as IntoPropValue<Option<AttrValue>>>
             <Option<&'static str> as IntoPropValue<Option<String>>>
             <Option<F> as IntoPropValue<Option<yew::Callback<I, O>>>>    // here
             <Option<String> as IntoPropValue<Option<AttrValue>>>
             <Option<std::rc::Rc<str>> as IntoPropValue<Option<AttrValue>>>

Edit: I'm trying other commands, but running into Github API limit since my personal access token is not being used:

Edit: What finally worked was:

rustup override set 1.56
TRYBUILD=overwrite cargo test --all-targets --workspace --exclude yew --exclude website-test --no-fail-fast

(note: --no-fail-fast prevents githup api rate limit early on from stopping the tests)

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.

Ah, the changelog test. Might have to introduce an easier way to not run it, since the anonymous gh-api easily hits the API limit :/ Thanks, this looks good now.

@WorldSEnder WorldSEnder merged commit ea8a530 into yewstack:master Mar 27, 2022
@finnbear finnbear deleted the 2553_support_closure_prop branch March 27, 2022 22:58
@finnbear finnbear restored the 2553_support_closure_prop branch March 27, 2022 22:58
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 ergonomics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component Callback documentation suggests non-existent conversion of closure to Callback
4 participants