-
Notifications
You must be signed in to change notification settings - Fork 379
Code generation and delegation to nested routes #1887
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
Draft
parsonsmatt
wants to merge
92
commits into
master
Choose a base branch
from
mattp/nested-route-discovery
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+3,538
−624
Conversation
This file contains hidden or 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
In order to call yesodDispatchNested, I need to provide a function `a -> Route site`. This function should be programmatically generated, and corresponds to the composed constructor. This requires that nested dispatch knows about the full `Route app`, but that's not a huge deal.
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.
Nested Route Separation
We have almost 2,000 total routes in our codebase now, with 1,100 of them on the top-level.
We have been leveraging nested routes to organize the routes and make certain things easier - namely, reducing the overall size of the
Route WebAppwhich makes pattern matching more efficient in compilation.However, compiling
mkYesodDatafor our application is a large bottleneck.Compilation takes a significant amount of time and much work must be redone, even when most of the routing information could have been saved.
One thing that would significantly help is the ability to separate out the sub-route datatypes, generate the instance for these separately, and refer to them elsewhere.
Another thing that would help is having a finer grained
YesodDispatchfacility - allowing us to, in tests, specify more precisely which parts of the route structure we actually need to deal with.Currently, we need almost 7k modules in order to compile a test that refers to
YesodDispatch, since this brings in the transitive dependencies of every web handler.With a finer grained
YesodDispatch, able to refer to route fragments, we'd be able to avoid depending on more of the website than we really need to.We expect this to yield significant benefits when we move more testing infrastructure to buck2 - right now, changing anything used by any part of the web app would require running all tests that exercise anything in the webapp!
With more granular
YesodDispatch, we gain the ability to only run tests on parts of the app that actually use the route structure in question.This PR is a breaking change, and so will be
yesod-core-0.2.0.0. When I tried this branch on our package, it required 0 modifications to upgrade - most of the breaking changes here are breaking "advanced power user" type functions, like the ability to customize theMkDispatchSettingsto have highly custom route behaviors. We don't use any of those, and I have no idea how widespread their use is. I will admit to finding them very difficult to use and adapt, which is part of why I ended up scrapping their use in theParseRoutegeneration (the class isn't even used).For the most part, though, all prior behavior remains unchanged. The performance of routing may be slightly worse -
YesodDispatchNestedreturns aMaybe Application(but with theRequestpulled out - only the "send response" side is in the Maybe) to support fallthrough. And if you enable fallthrough, then the app will do more checking (proportional to the number of routes and nested routes) rather than early-404ing. But IMO for perf, we're better off coming up with a trie-based router than trying to optimize this one much more.For an example of the testing facilities this provides, see parsonsmatt/hspec-yesod#7 - particularly, this file has an introduction to how the tests are separated and defined.
Fix for #1880
Before submitting your PR, check that you've:
@sincedeclarations to the Haddocks for new, public APIsAfter submitting your PR: