Skip to content
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

Make accountant secretary-agnostic #22

Merged
merged 3 commits into from
Feb 20, 2016
Merged

Conversation

arohner
Copy link
Contributor

@arohner arohner commented Feb 19, 2016

I wanted to use accountant, but I use bidi instead of secretary, so I made it un-opinionated about how routing is performed.

@@ -33,10 +33,41 @@ All you have to do to get Accountant working is the following:
(ns your-app-ns
(:require [accountant.core :as accountant]))

(accountant/configure-navigation!)
(accountant/configure-navigation! {:nav-handler (fn [path] ...) :path-exists? (fn [path] ...)})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a question: is there a way we could make macros for :nav-handler and :path-exists? and then just allow a keyword option? i.e.:

(accountant/configure-navigation! :secretary)

This sort of thing seems to work very well for secretary, but maybe less well with Bidi (since the example functions you provided include references to the global atom in addition to the function).

Maybe we could do something instead of keywords where we chose to do either:

{:router :secretary}

or

{:router :bidi
 :path-exists? (fn [path] ...)
 :nav-handler (fn [path] ...)}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're using secretary, there's exactly one way to do it. That's not true in bidi, because bidi is barely more than a pattern matching library. Also note than in bidi, I have to close over app-routes. There are other routers, like silk which are closer in approach to bidi than secretary (pure, functional, unopinionated).

Having to support both :secretary and the manual way is needless sugar, IMO. Also, you'd need a way to conditionally require secretary, so non-secretary users don't pull it into their CLJS builds.

@venantius
Copy link
Owner

I really like this. I might build in some convenience stuff in the future for people using Secretary, but this is a great first step to making it support multiple routing libraries.

venantius added a commit that referenced this pull request Feb 20, 2016
Make accountant secretary-agnostic
@venantius venantius merged commit 4a8149a into venantius:master Feb 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants