-
Notifications
You must be signed in to change notification settings - Fork 64
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
v2.0.0 (DO NOT MERGE) #50
base: master
Are you sure you want to change the base?
Conversation
This looks great! I have one scenario in mind that currently requires some gymnastics. I'm wondering how it could be improved or if I'm completely overlooking it. The scenario is the following: a page requires some extra dispatching for logged users, on top of regular dispatching applied to anonymous users. Because checking for logged users take some time the dispatching is done in 2 steps. Also this check is performed via JS and does not impact the URL. Currently I'm handling this via |
@jeluard Hmm... I don't think there is anything special about the new API that would apply to that problem specifically. OTOH you could create middleware which would check if the user is logged in and
Since Does that help? |
@jeluard This might be a better illustration (defn wrap-add-login-status
[handler]
(fn [req]
(handler (assoc req :logged-in? (logged-in?)))))
(defn wrap-authorized
[handler]
(fn [req]
(if (:logged-in? req)
(handler (assoc-in req [:params :secret-data] 42))
;; Alter the action.
(handler (assoc-in req [:route :action] unauthorized-action))))) FWIW, you could build a completely custom dispatching function. Really there's not a lot to it. You create some |
Sure this looks great! I still have to wrap my head around it but definitively looks like the necessary flexibility is there. |
Just wondering, what is the status of this? I'm looking at silk and secretary for my new project and would love to use secretary2 for the comparison. |
@stephanos I just need to write the documentation for it and patch one more thing. Other than that it's pretty much ready to go. We're using this branch in production right now with no problems. Look at the README on this branch to get the current version. The biggest difference between Silk and Secretary is that Silk supports Clojure. This can be nice if your project has both a front-end and a back-end. Since most of the projects we work on at work do not do this I've never had the motivation to add Clojure support to Secretary (although it really wouldn't require that much more effort). A lot of the problems that were brought up regarding Secretary on the mailing list now have largely been corrected on this branch. |
@stephanos I should clarify that there are other design differences between Silk and Secretary but in my opinion there isn't a lot of contrast between the two. If you find that you like Silk more than Secretary, I'd like to hear your thoughts. |
Conflicts: project.clj src/secretary/core.cljs test/secretary/test/core.cljs
Fix extend type syntax
Hello @noprompt. What is the status of this pull request? |
@dgellow It's a bit stale at the moment. 😞 |
And what is the current state of the pull request? Ultimately on what parts can we work to help you? In december 2014 you wrote:
Is it still the case (just missing some documentation and a patch)? |
@dgellow Sort of. There have been some commits to master since this branch was created, some of it may need to be applied to this branch if it's relevant. Also I think there might be some work around If you're serious please send me a pull request for this branch. After review, I'd be happy to release a new version. |
@noprompt, I submitted this pull request yesterday that resolves all the conflicts with master and updates all of secretary's dependencies. Do you remember what the remaining work around |
@zonotope good job! 👍 |
@zonotope This is awesome, thanks for stepping up. You know, I don't necessarily think that tl;dr I'd be happy to take it out for now for the sake of getting an alpha out. |
@noprompt I don't think we need a I opened another branch and made some tweaks to the macro definitions First, I made some changes to Also, I added a What do you think of those changes? |
@zonotope These additions are great. I especially like what you've done with I think I can get on board discarding the alpha and using the version format with the identifier ( How does the documentation look? It's been a while and I don't remember how far along I got syncing that up. If that looks good I think all that's necessary from me is to pull this down and try it out. Also, if you're cool with it, I think this earns you contributor status once it's merged. @gf3 what do you think? |
@zonotope Sorry I dropped the ball on this. I guess I can take a look at the documentation and get this merged soon. |
Version 2.0.0 is primarily focused on addressing issues #16 and #41 as well as cleaning up some of the API. As with any major release, this means breaking changes.
Solution to #16
Solving #16 required the removal of the global
*routes*
atom. Removing it had a dramatic impact on the code consequentlyRoute
([pattern action]
) which specifies default implementations for all of the routing protocols. It also implementsIFn
which defaults torender-route
.make-route
function as a convenience for creating routes. This is now whatdefroute
uses reducing the complexity of the macro.locate-route
andlocate-route-value
. Since routes are no longer stored in a container managed by Secretary, these can be implemented by the library consumer.Solution to #41
Solving #41 required solving #16 in order to create customer dispatchers.
There is a new data typeThis meansDispatcher
([routes handler]
) which is simply a container for housing routes and a handler function([routes dispatch-val]
). TheDispatcher
type implements a unaryIFn
which takes a dispatch value and delegates to the handler.dispatch!
. We simply do not need this function any longer.uri-dispatcher [routes handler]
which returnsan instance ofa unary function ofDispatcher
[uri]
that does most of the work the originaldispatch!
function did with the exceptionhandler
is a ring style handler. Why is it calleduri-dispatcher
? Because that is what it's designed to do: dispatch actions based on URIs. There is nothing in the routing protocols that say patterns must apply to URIs. Ainput-dispatcher
might be used to dispatch against the values of<input>
element.*config*
. This was a "fix it for now" artifact. The problem of prepending/removing prefixes and ensuring leading slashes shouldn't be a concern for Secretary. These are problems which are better addressed by the library consumer. For example, to ensure a URI has a leading/
one can create a function whichcreates a new instance ofwrapsDispatcher
wrapping theDispatcher
return fromuri-dispatcher
. Prepending prefixes toRoute
s can be handled by overridingIFn
with an implementation that does so.Clean up
In the spirit of Ring, query-parameter encoding/decoding and serialization has been moved from
secretary.core
tosecretary.codec
.Won't fix?
I do not believe, although I'm open to conversation, we need to implement #18,
defroutes
, or other concepts from Compojure. My rational, again, being that these are library consumer decisions and are trivial to implement when needed.Summary
This version focuses on trimming down the parts of the API we simply do not need and creating the right abstractions and functions such that solving client-side routing issues is easy and flexible. These changes will definitely break existing applications which use Secretary and upgrading will require extra on behalf of the consumer (although hopefully not much). Overall, I hope many of these changes will be welcome.