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

async_proxy example #30

Merged
merged 4 commits into from
Jul 30, 2022
Merged

async_proxy example #30

merged 4 commits into from
Jul 30, 2022

Conversation

alilee
Copy link
Contributor

@alilee alilee commented Jul 27, 2022

Too much? Also, delete doesn't work... Would love your thoughts, even if you don't want this for any reason. Happy to iterate it if there is something useful.

@intendednull
Copy link
Owner

intendednull commented Jul 28, 2022

Looks great! Thank you for contributing.

Also, delete doesn't work...

After testing on my side, it looks like this line is crashing because it unwraps the value. Reason being the use_selector hook runs at the exact moment state changes (like all subscribers, not tied to component lifecycle). So when the entry is deleted, the selector runs, but it can't find the entry that was just removed, so it panics.

I think this is a great example! Good to have more complex code to reference, especially with net logic. If we can get all the unwraps handled properly, I'd say it'll be ready to merge!

@alilee
Copy link
Contributor Author

alilee commented Jul 28, 2022

I got to the same point. I don't know why it generates the Time component though - there isn't an entry in the State to iterate over so the unwrap should be safe. This seems like a Yew order-of-execution problem/defect? Is the fix to tell Yew to destroy the child the same time we State::delete?

@alilee
Copy link
Contributor Author

alilee commented Jul 28, 2022

Ah ok, I get it.

@alilee
Copy link
Contributor Author

alilee commented Jul 28, 2022

There is still a defect which I don't understand: missing timezone when user pushes buttons for Mel, Adl, and Uto, then deletes Mel. The correspondence between the keys and Time objects is getting messed up.

@alilee
Copy link
Contributor Author

alilee commented Jul 28, 2022

The smallest case is deleting the last item of any two.

@intendednull
Copy link
Owner

intendednull commented Jul 28, 2022

This seems like a Yew order-of-execution problem/defect? Is the fix to tell Yew to destroy the child the same time we State::delete

Yeah, a little tricky because the selector doesn't run when you'd expect it to. Maybe there is room for improvement here

@intendednull
Copy link
Owner

Try adding a unique key to each element. Sometimes Yew gets confused with lists of similar elements

@alilee
Copy link
Contributor Author

alilee commented Jul 28, 2022

Makes sense and has changed the result, but hasn't fixed it. When I get a moment, I'll try to repro it without yewdux.

@intendednull
Copy link
Owner

Make sure it's at top level. This seems to fix it for me, though maybe you're talking about a different issue.

    html! {
        <tr key={timezone.clone()}>
            <th> { timezone } </th>
            <td> { datetime } </td>
            <td> { format!("{:?}", status) } </td>
            <td> <button onclick={refresh}> { "Refresh" } </button> </td>
            <td> <button onclick={delete}> { "Delete" } </button> </td>
        </tr>
    }

@alilee
Copy link
Contributor Author

alilee commented Jul 29, 2022

I'm still talking about the delete of last in list leaving missing timezone issue - video

@intendednull
Copy link
Owner

intendednull commented Jul 29, 2022

Oh yeah, I think I know what the issue is. Adjusting the selector like this should fix it:

use_selector_with_deps(|state: &State, timezone| state.get(timezone), timezone);

Basically the timezone prop is changing for the component, but the selector is using the old value. We need to tell the selector to update whenever timezone changes as well.

@alilee
Copy link
Contributor Author

alilee commented Jul 29, 2022

Bullseye! I need to read about that. This is probably good to go, but do you think the example would be clearer if I back out the unwrap handling, now they should be unreachable?

@alilee
Copy link
Contributor Author

alilee commented Jul 29, 2022

No, I think it's fine. Happy to merge?

@intendednull
Copy link
Owner

This is good, even if they're unreachable in this example, it's nice to show how you would manage otherwise. Also helps others avoid the trap we fell into lol

@intendednull intendednull merged commit ba003ac into intendednull:master Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants