Add url field to routes to build path and use it everywhere
#1035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Approach looks good to me 👌 |
LukasKalbertodt
force-pushed
the
route-url-builders
branch
from
22be7b5
to
8e1de85
Compare
LukasKalbertodt
changed the title
url field to routes to build pathurl field to routes to build path and use it everywhere
|
Ok I did that for every route and tried to use those functions everywhere. I'm sure I forgot a few places, but that's fine, we can still change them whenever we notice them. The diff is huge. It gets a lot smaller by "hide whitespace changes". But its still large and annoying to review as its just a refactor. My suggestion is that someone spends at most 5min on this as reviewing this in detail is not a good use of our time IMO. |
github-actions
bot
added
the
status:conflicts
This comment was marked as resolved.
This comment was marked as resolved.
The previous commit once again ran into one of these annoying import cycles that only error at runtime. Once again I don't have the time to find a proper solution here, so I just reverted a tiny bit of the last commit.
LukasKalbertodt
force-pushed
the
route-url-builders
branch
from
495675e
to
12adaef
Compare
github-actions
bot
removed
the
status:conflicts
|
As suggested, I only glanced over the code changes and didn't read everything in detail. But I did a quick test and everything appears to be working. Since Julian also gave his blessing, I'm merging this now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Will fix #844
I considered multiple approaches to this. I think having an
urlfield on the route object itself is the nicest, at least for using it.DirectVideoRoute.url({ videoid: ... })reads very clearly to me. But it wasn't that straight forward to find a way that plays nicely with Typescript.Adding the
urlfield toRouteand keeping everyFooRouteas typeRouteis tricky because (a) in the beginning, not every route will have anurlfield, and (b) the url builder has different arguments per route, making it generic and more complicated. We certainly want to involveRoutesomehow such that thematchfunction is well typed without the need to annotate the type of its argument. So yeah, this is what I came up with.urleven the right term? It always just returns a path. Which is an URL... or URI, I can never tell.Also: hide whitespace changes in githubs diff view when looking at this!
Obviously this is not nearly done, just a tiny proof of concept to get feedback. Which I'd like to get from @JulianKniephoff as well.