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

Add support for struct Routes #434

Merged
merged 4 commits into from
Jun 26, 2022
Merged

Conversation

arctic-hen7
Copy link
Contributor

This solves a problem I had with Sycamore last year: that Route can only be an enum, meaning additional custom information can't be easily attached to the router system. Perseus required this, and did a very hacky workaround that both reduced routing performance there and made for some really weird semantics. With framesurge/perseus#151 though, that workaround it no longer feasible, so this makes Route be able to be a struct, and match_route now takes &self.

To achieve this, I've broken out the Router into a Router and a RouterBase, with the former being a wrapper around the latter that simply calls it with R::default(), which is now derived automatically in enums as the #[not_found] route. All tests pass, and I'm pretty sure I've covered everything, but a review to make sure I haven't missed something would be greatly appreciated!

I haven't done anything with StaticRouter, which already takes a specific Route instance. RouterBase now mimics that, and Router just abstracts over it (meaning this isn't a breaking change).

Closes #244.

This makes artic-hen7/perseus#151 and more advanced custom router
patterns feasible.
This stuffed up some tests, and I'm fairly certain it actually isn't necessary.
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #434 (8569b17) into master (215a015) will decrease coverage by 0.21%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   64.97%   64.75%   -0.22%     
==========================================
  Files          52       52              
  Lines        8268     8385     +117     
==========================================
+ Hits         5372     5430      +58     
- Misses       2896     2955      +59     
Impacted Files Coverage Δ
packages/sycamore-router/src/router.rs 25.17% <9.09%> (-1.83%) ⬇️
packages/sycamore-router/src/lib.rs 96.95% <95.74%> (+0.18%) ⬆️
packages/sycamore-router-macro/src/route.rs 89.76% <100.00%> (+0.34%) ⬆️
packages/sycamore-web/src/lib.rs 2.38% <0.00%> (-1.33%) ⬇️
packages/sycamore/src/builder.rs 21.93% <0.00%> (-1.18%) ⬇️
packages/sycamore-macro/src/view/ir.rs 100.00% <0.00%> (ø)
packages/sycamore-macro/src/view/parse.rs 96.89% <0.00%> (+0.08%) ⬆️
packages/sycamore-macro/src/view/codegen.rs 97.34% <0.00%> (+1.12%) ⬆️

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 215a015...8569b17. Read the comment docs.

@arctic-hen7 arctic-hen7 marked this pull request as draft June 11, 2022 05:29
@arctic-hen7 arctic-hen7 marked this pull request as ready for review June 11, 2022 05:33
@arctic-hen7
Copy link
Contributor Author

Note that only Cloneable data will be able to be stored in the Route right now, since it takes &self rather than self. if it takes the latter though, that stuffs up some lifetimes in another part of the code, which I'm not completely sure how to quickly fix. For my purposes, this is fine, but it might be worth revisiting in future.

Copy link
Member

@lukechu10 lukechu10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry don't have time for a full review right now. I'll try to get to this soon.

packages/sycamore-router-macro/src/route.rs Outdated Show resolved Hide resolved
@arctic-hen7
Copy link
Contributor Author

No problem, Perseus can run off my fork in the meantime.

@arctic-hen7
Copy link
Contributor Author

@lukechu10, would it be possible to merge this fairly soon and get a new beta release out? I'd like to make a new release of Perseus fairly soon to field test the removal of .perseus/, which depends on this.

@lukechu10
Copy link
Member

Sorry for taking so long to get to this. I'm planning to do a refactor of the router API for the final 0.8 release but I'll merge this in for now to unblock you.

@lukechu10 lukechu10 merged commit a865f15 into sycamore-rs:master Jun 26, 2022
@lukechu10
Copy link
Member

Released v0.8.0-beta.7!

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.

Support implementing Route on structs
2 participants