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

Core routing refactor #211

Merged
merged 160 commits into from Mar 29, 2017

Conversation

Projects
None yet
2 participants
@bryanp
Member

bryanp commented Dec 5, 2016

It mostly works, but there are quite a few things to cleanup (see TODO comments). I summarized my thoughts in this discussion. Feel free to chime in!

  • feature tests
  • call handlers in current router first
    • and then only call parents, not routers outside of the current ancestry
  • request format matchers
  • new within method
  • configurable router priority
  • global handlers
  • inline api docs

Remaining Test Cases

  • that respond_to halts all on its own (int)
  • sending an argument of an unsupported type (int)
  • deleting cookie keys during request processing (int)
  • routing template definition (int)
  • know events defined on Pakyow::App (unit)
  • that Pakyow::App.reset calls super (unit)
  • Pakyow::App#initialize does the right things (unit)
  • config options / default values (unit)
  • Pakyow::App#call creates a new Pakyow::Controller (unit)
  • that requir_recursive is called when Pakyow::App loads (unit)
  • known events on Pakyow::Controller (unit)
  • Pakyow::Controller delegators, accessors (unit)
  • Pakyow::Controller.process does the right thing (unit)
  • Pakyow::Controller#initialize does the right thing (unit)
  • Pakyow::Router supported http methods, default extensions (unit)
  • Pakyow::Router delegators, accessors (unit)
  • Pakyow::Router initialization (unit)
  • router method_missing / respond_to_missing? (unit)

bryanp and others added some commits Dec 5, 2016

Initial routing refactor
It mostly works, but there are quite a few things to cleanup. Also needs
tests,  inline api documentation, and user-facing documentation.
Controller, Inline Handlers, & Error Mapping
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.
Process each request in a new router instance
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.
Remove Static middleware
This will ultimately be replaced with pakyow-assets.

bryanp added some commits Mar 23, 2017

Better separation of routing concerns
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/
@bryanp

This comment has been minimized.

Show comment
Hide comment
@bryanp

bryanp Mar 27, 2017

Member

🎉 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.

Member

bryanp commented Mar 27, 2017

🎉 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 bryanp changed the title from [WIP] Routing refactor to Refactor routing for 1.0 Mar 27, 2017

@bryanp bryanp changed the title from Refactor routing for 1.0 to 1.0 core routing refactor Mar 27, 2017

@jphager2

This comment has been minimized.

Show comment
Hide comment
@jphager2

jphager2 Mar 27, 2017

Contributor

@bryanp, thats okay by me. I will try to go through your recent commits today. Good work getting this done!

Contributor

jphager2 commented Mar 27, 2017

@bryanp, thats okay by me. I will try to go through your recent commits today. Good work getting this done!

@jphager2

@bryanp, I think this is great work. Really appreciate the work on the docs as well. Good job getting this done!

Show outdated Hide outdated pakyow-core/lib/pakyow/core/app.rb
Show outdated Hide outdated pakyow-core/lib/pakyow/core/app.rb
# Pakyow::App.router CustomMatcher.new do
# end
#
# Custom matchers can also make data available in +params+ by implementing

This comment has been minimized.

@jphager2

jphager2 Mar 28, 2017

Contributor

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)

@jphager2

jphager2 Mar 28, 2017

Contributor

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)

This comment has been minimized.

@bryanp

bryanp Mar 28, 2017

Member

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.

@bryanp

bryanp Mar 28, 2017

Member

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.

Show outdated Hide outdated pakyow-core/lib/pakyow/core/router.rb
Show outdated Hide outdated pakyow-core/lib/pakyow/core/router.rb
Show outdated Hide outdated pakyow-core/lib/pakyow/core/router.rb
Show outdated Hide outdated pakyow-core/lib/pakyow/core/router.rb
if route.is_a?(Proc)
context.instance_exec(&route)
else
context.__send__(route)

This comment has been minimized.

@jphager2

jphager2 Mar 28, 2017

Contributor

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?

@jphager2

jphager2 Mar 28, 2017

Contributor

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?

This comment has been minimized.

@bryanp

bryanp Mar 28, 2017

Member

__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.

@bryanp

bryanp Mar 28, 2017

Member

__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|

This comment has been minimized.

@jphager2

jphager2 Mar 28, 2017

Contributor

Interesting. Do you have a specific use case for this that you can share as an example? Will we need docs for this?

@jphager2

jphager2 Mar 28, 2017

Contributor

Interesting. Do you have a specific use case for this that you can share as an example? Will we need docs for this?

This comment has been minimized.

@bryanp

bryanp Mar 28, 2017

Member

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.

@bryanp

bryanp Mar 28, 2017

Member

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.

This comment has been minimized.

@jphager2

jphager2 Mar 29, 2017

Contributor

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.

@jphager2

jphager2 Mar 29, 2017

Contributor

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

This comment has been minimized.

@jphager2

jphager2 Mar 28, 2017

Contributor

Just out of curriosity, why not refine Dir instead of its singleton class?

@jphager2

jphager2 Mar 28, 2017

Contributor

Just out of curriosity, why not refine Dir instead of its singleton class?

This comment has been minimized.

@bryanp

bryanp Mar 28, 2017

Member

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.

@bryanp

bryanp Mar 28, 2017

Member

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.

This comment has been minimized.

@jphager2

jphager2 Mar 29, 2017

Contributor

Cool. Thanks for the explanation. Still haven't used refinements too often. Quite useful though.

@jphager2

jphager2 Mar 29, 2017

Contributor

Cool. Thanks for the explanation. Still haven't used refinements too often. Quite useful though.

@bryanp bryanp merged commit 62d94df into environment Mar 29, 2017

@bryanp bryanp deleted the routing-refactor branch Mar 29, 2017

@bryanp bryanp changed the title from 1.0 core routing refactor to Core routing refactor Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment