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 destructuring async props #419

Merged
merged 6 commits into from
May 10, 2022

Conversation

mc1098
Copy link
Contributor

@mc1098 mc1098 commented May 10, 2022

Fixes #410

This PR fixes support in the #[component] macro for destructured struct patterns of props.

I added a single test to trybuild using the existing prop struct with generics and lifetimes to show it handles those too :)

I started trying to do what I think was discussed here only to find that trying to match on fields that themselves might also be destructured was a rabbit hole I didn't want to go down :)

So instead this change simply alters the new sync function that we create to wrap around the async one - it takes the type from the destructured pattern and converts it into a syn::PatType (i.e. props: MyProps) then we just have to add this ident to the args we supply the async function and the pattern the user uses will work if valid and all the errors associated with incorrect patterns will be well formed from rustc.

I added the more eager parser check for the first argument to be syn::Ident instead of the unwrap that was there before as this should give a better error message that will be consistent across both types of components.

Moved most of the change to its own function as it got bit messy and used a struct for named fields so it is hopefully more clear :)

mc1098 added 5 commits May 10, 2022 00:20
Adds a test for destructed struct pattern using an async component that
is currently failing just like in sycamore-rs#410.
This change fixes `#[component]` proc macro generation to support the
struct destructured pattern. This was different than non-async structs as
async structs are converted into non-async structs.
We can unwrap so avoid the nesting with map and just unwrap immediately
and perform the match on the prop argument.
Replace the simple destructuring test with the more complicated one to
show that destructuring works even with lifetimes and generics
included.
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #419 (9493584) into master (49135bd) will increase coverage by 0.23%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   64.48%   64.72%   +0.23%     
==========================================
  Files          49       49              
  Lines        7958     8022      +64     
==========================================
+ Hits         5132     5192      +60     
- Misses       2826     2830       +4     
Impacted Files Coverage Δ
packages/sycamore-macro/src/component/mod.rs 96.01% <90.47%> (-1.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49135bd...9493584. Read the comment docs.

@danielalvsaaker
Copy link
Contributor

danielalvsaaker commented May 10, 2022

This looks very nice, but I think you're doing more work than necessary. Here's my proposed solution. It's okay to reuse the destructured struct as input to the inner async function.

Actually, ignore the previous statement. It won't handle partially destructured assignments. Your solution looks good!

@mc1098
Copy link
Contributor Author

mc1098 commented May 10, 2022

This looks very nice, but I think you're doing more work than necessary. Here's my proposed solution. It's okay to reuse the destructured struct as input to the inner async function.

Actually, ignore the previous statement. It won't handle partially destructured assignments. Your solution looks good!

Yeah, I don't know how likely someone will use partial destructured patterns for props but good to include it if we can :)

Also, I'm sorry! I didn't realise you were working on the issue and now I feel like I've stepped on your toes - if you want I can close this PR and allow you to continue working on the issue.

Makes the `AsyncComponentWithPropDestructuring` function actually async..
@danielalvsaaker
Copy link
Contributor

No, that's okay, I was just testing an alternative solution. 🙂

@lukechu10
Copy link
Member

Looks great thanks!

@lukechu10 lukechu10 merged commit ffa26f7 into sycamore-rs:master May 10, 2022
@mc1098 mc1098 deleted the destructuring-async-props branch May 10, 2022 16:05
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.

Allow destructuring in the component signature
3 participants