Core routing refactor #211
Conversation
| if @path.include?(":") | ||
| # replace named parameters with a named capture | ||
| reqex_path = @path.split("/").map { |segment| | ||
| if segment.include?(":") |
There was a problem hiding this comment.
Should this be the same condition as on line 45 in populated_path?
There was a problem hiding this comment.
Yep, good catch! Actually, this might be prime for a refactor. We could add a method that yields each parameter segment and lets us provide a new value. I imagine it would be used here like this:
regex_path = replace_param_segments(@path) do |segment|
'(?<' + segment[1..-1] + '>(\w|[-.~:@!$\'\(\)\*\+,;])*)'
endAnd back in populated_path like this:
replace_param_segments(parameterized_path) do |segment|
params[segment[1..-1].to_sym]
end
|
Just a thought for later: https://github.com/pakyow/pakyow/blob/routing-refactor/pakyow-core/lib/pakyow/core/app.rb#L177 Maybe it would be more readable to use some constant here (e.g. Router::FOUND or Router::MATCHED) |
It mostly works, but there are quite a few things to cleanup. Also needs tests, inline api documentation, and user-facing documentation.
Routes are no longer called in context of App. Instead, a new Controller instance is created on every request. This commit also introduces inline handlers and error mapping.
This commit changes the context in which routes are evaluated. Rather than in a `Controller` instance, evaluation now occurs in a new `Router` instance for each request. These changes let us use routers much like normal classes. Functions are now just methods that are called on a router instance. Modules can be included into routers that define new methods, just like any PORO.
This will ultimately be replaced with pakyow-assets.
Controllers are now responsible for everything related to the request/response lifecycle, including handing off from one router to another and calling the matching route for a request. This makes router more or less a container for holding routes and finding a match. Routers and Routes now support generic matchers, rather than a path. When passed a string path, a regex matcher will automatically be created. Routers and Routes can also be defined with Regexp objects, or any custom object that implements a `match` method. Concerns of template expansion have been refactored out of Router completely. No more heartburn over that \o/
|
🎉 Holy shit it's done. Leaving it open for review for a bit. Planning to merge Tuesday. I'll likely squash before merging, because several (likely the majority) of the commits here add no value. If @jphager2 or anyone else has any hesitations with this decision, please speak up. |
|
@bryanp, thats okay by me. I will try to go through your recent commits today. Good work getting this done! |
pakyow-core/lib/pakyow/core/app.rb
Outdated
| # | ||
| after :configure do | ||
| if config.session.enabled | ||
| builder&.use config.session.object, config.session.options |
There was a problem hiding this comment.
Is it really okay to use &. here. Wouldn't it be a problem if builder isn't set?
There was a problem hiding this comment.
Yeah okay it's kind of a smell. Removed
pakyow-core/lib/pakyow/core/app.rb
Outdated
| def router | ||
| Router.instance | ||
| if config.protection.enabled | ||
| builder&.use(Rack::Protection, except: config.session.enabled ? [] : [:session_hijacking, :remote_token]) |
| # Pakyow::App.router CustomMatcher.new do | ||
| # end | ||
| # | ||
| # Custom matchers can also make data available in +params+ by implementing |
There was a problem hiding this comment.
Interesting. Do you already have a use case for this? I guess this can be used for constraints (e.g. that a path includes an accepted locale)
There was a problem hiding this comment.
I have come up with a couple contrived uses, but nothing that specifically led me to add this feature. Supporting custom matchers is really just a side-effect of our implementation.
| # Pakyow::App.router CustomMatcher.new do | ||
| # end | ||
| # | ||
| # When defined on a Router, custom matchers should also implement +sub+, |
There was a problem hiding this comment.
Should this be included in the example above?
There was a problem hiding this comment.
Ah, good call. I'll add it.
There was a problem hiding this comment.
Actually, this entire thing is incorrect. We don't currently support path modification from custom matchers. I decided that there wasn't a great reason to support this for now.
| def initialize | ||
| @sets = {} | ||
| router = self | ||
| (class << Pakyow; self; end).send(:define_method, :Router) do |path, **hooks| |
There was a problem hiding this comment.
Is equivalent to this (out of curriosity)?
Pakyow.singleton_class.class_eval do
define_method :Router do |path, **hooks|
router.Router(path, **hooks)
end
endThere was a problem hiding this comment.
Tried it and yes, it appears to be functionally equivalent. Will update
| # end | ||
| # end | ||
| # | ||
| # @api public |
There was a problem hiding this comment.
I am just wondering if this really be public? Should we expect this to happen: router.method_missing?
There was a problem hiding this comment.
You're right, I'll make it @api private.
| return parent.find_router_by_name(names) if parent | ||
|
|
||
| first_name = names.shift | ||
| (routers || [self].concat(state.instances)).each do |router| |
There was a problem hiding this comment.
I think the world needs a blog post on stateful =).
| if route.is_a?(Proc) | ||
| context.instance_exec(&route) | ||
| else | ||
| context.__send__(route) |
There was a problem hiding this comment.
I noticed that sometimes send is used and here __send__. I prefer __send__, but maybe it would be best to only do it one way. Which is your preference?
There was a problem hiding this comment.
__send__ is definitely my preference, given that I spent an hour or so figuring out why this wasn't working right. Let's standardize around __send__ in the framework.
| end | ||
|
|
||
| def reprioritize! | ||
| @instances.sort! { |a, b| |
There was a problem hiding this comment.
Interesting. Do you have a specific use case for this that you can share as an example? Will we need docs for this?
There was a problem hiding this comment.
Ah, router priority isn't defined! I'll add it.
The case that led to router priority is when a router defines a wildcard route. You usually want the wildcard to be the last route matched, because it's usually some sort of fallback. Using router priority, you can designate the router containing the wildcard as low priority.
There was a problem hiding this comment.
Nice. It's an interesting way to handle this problem. In Rails I think they add some methods for prepending/appending routes. Priorities is much better. Gives you more transparency and more control.
| module WalkDir | ||
| DOT = ".".freeze | ||
|
|
||
| refine Dir.singleton_class do |
There was a problem hiding this comment.
Just out of curriosity, why not refine Dir instead of its singleton class?
There was a problem hiding this comment.
If we just did refine Dir it would add walk as an instance method. But we want it to be available on the class as Dir.walk.
There was a problem hiding this comment.
Cool. Thanks for the explanation. Still haven't used refinements too often. Quite useful though.
It mostly works, but there are quite a few things to cleanup (see
TODOcomments). I summarized my thoughts in this discussion. Feel free to chime in!withinmethodRemaining Test Cases
Pakyow::App(unit)Pakyow::App.resetcallssuper(unit)Pakyow::App#initializedoes the right things (unit)Pakyow::App#callcreates a newPakyow::Controller(unit)requir_recursiveis called whenPakyow::Apploads (unit)Pakyow::Controller(unit)Pakyow::Controllerdelegators, accessors (unit)Pakyow::Controller.processdoes the right thing (unit)Pakyow::Controller#initializedoes the right thing (unit)Pakyow::Routersupported http methods, default extensions (unit)Pakyow::Routerdelegators, accessors (unit)Pakyow::Routerinitialization (unit)method_missing/respond_to_missing?(unit)