From c86f042f54ee9200b034769635a7ecab46432939 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 15 Mar 2019 17:33:46 +0200 Subject: [PATCH 1/3] Implement #238, handle trailing slashes in frontend match-by-path --- .../reitit-frontend/src/reitit/frontend.cljs | 24 +++++- test/cljs/reitit/frontend/core_test.cljs | 86 +++++++++++++++++++ 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/modules/reitit-frontend/src/reitit/frontend.cljs b/modules/reitit-frontend/src/reitit/frontend.cljs index 8145404a1..5ff4f09ad 100644 --- a/modules/reitit-frontend/src/reitit/frontend.cljs +++ b/modules/reitit-frontend/src/reitit/frontend.cljs @@ -1,5 +1,6 @@ (ns reitit.frontend (:require [clojure.set :as set] + [clojure.string :as str] [reitit.coercion :as coercion] [reitit.coercion :as rc] [reitit.core :as r]) @@ -16,10 +17,18 @@ (defn match-by-path "Given routing tree and current path, return match with possibly - coerced parameters. Return nil if no match found." + coerced parameters. Returns nil if no match found." [router path] - (let [uri (.parse Uri path)] - (if-let [match (r/match-by-path router (.getPath uri))] + (let [uri (.parse Uri path) + path (.getPath uri)] + (if-let [match (or (r/match-by-path router path) + (if-let [trailing-slash-handling (:trailing-slash-handling (r/options router))] + (if (str/ends-with? path "/") + (if (not= trailing-slash-handling :add) + (r/match-by-path router (subs path 0 (dec (count path))))) + (if (not= trailing-slash-handling :remove) + (r/match-by-path router (str path "/"))))))] + ;; User can update browser location in on-navigate call using replace-state (let [q (query-params uri) match (assoc match :query-params q) ;; Return uncoerced values if coercion is not enabled - so @@ -40,7 +49,14 @@ (defn router "Create a `reitit.core.router` from raw route data and optionally an options map. - Enables request coercion. See [[reitit.core/router]] for details on options." + Enables request coercion. See [[reitit.core/router]] for details on options. + + Additional options: + + | key | description | + | -------------|-------------| + | :trailing-slash-handling | TODO | + " ([raw-routes] (router raw-routes {})) ([raw-routes opts] diff --git a/test/cljs/reitit/frontend/core_test.cljs b/test/cljs/reitit/frontend/core_test.cljs index 200090883..1a56934ba 100644 --- a/test/cljs/reitit/frontend/core_test.cljs +++ b/test/cljs/reitit/frontend/core_test.cljs @@ -104,3 +104,89 @@ (capture-console (fn [] (rf/match-by-name! router ::foo {})))))))))) + + +(deftest trailing-slash-handling-test + (testing ":both" + (let [router (r/router ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] + {:trailing-slash-handling :both})] + (is (= (r/map->Match + {:template "/foo" + :data {:name ::foo} + :path-params {} + :query-params {} + :path "/foo" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/foo/") + (rf/match-by-path router "/foo"))) + + (is (= (r/map->Match + {:template "/bar/" + :data {:name ::bar} + :path-params {} + :query-params {} + :path "/bar/" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/bar/") + (rf/match-by-path router "/bar"))) )) + + (testing ":add" + (let [router (r/router ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] + {:trailing-slash-handling :add})] + (is (= (r/map->Match + {:template "/foo" + :data {:name ::foo} + :path-params {} + :query-params {} + :path "/foo" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/foo"))) + (is (nil? (rf/match-by-path router "/foo/"))) + + (is (= (r/map->Match + {:template "/bar/" + :data {:name ::bar} + :path-params {} + :query-params {} + :path "/bar/" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/bar/") + (rf/match-by-path router "/bar"))))) + + (testing ":remove" + (let [router (r/router ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] + {:trailing-slash-handling :remove})] + (is (= (r/map->Match + {:template "/foo" + :data {:name ::foo} + :path-params {} + :query-params {} + :path "/foo" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/foo/") + (rf/match-by-path router "/foo"))) + + (is (= (r/map->Match + {:template "/bar/" + :data {:name ::bar} + :path-params {} + :query-params {} + :path "/bar/" + :parameters {:query {} + :path {}}}) + (rf/match-by-path router "/bar/"))) + (is (nil? (rf/match-by-path router "/bar")))))) From 54a26aded7f5252afb0a425b71adf05fd9a450f2 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 15 Mar 2019 17:37:13 +0200 Subject: [PATCH 2/3] . --- modules/reitit-frontend/src/reitit/frontend.cljs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/reitit-frontend/src/reitit/frontend.cljs b/modules/reitit-frontend/src/reitit/frontend.cljs index 5ff4f09ad..dadbe54ca 100644 --- a/modules/reitit-frontend/src/reitit/frontend.cljs +++ b/modules/reitit-frontend/src/reitit/frontend.cljs @@ -23,6 +23,8 @@ path (.getPath uri)] (if-let [match (or (r/match-by-path router path) (if-let [trailing-slash-handling (:trailing-slash-handling (r/options router))] + ;; TODO: Maybe the original path should be added under some key in match, + ;; so it is easy for user to see if trailing slash "redirect" happened? (if (str/ends-with? path "/") (if (not= trailing-slash-handling :add) (r/match-by-path router (subs path 0 (dec (count path))))) From 5ab3e26f16feb173ef46d33a48172453ae876868 Mon Sep 17 00:00:00 2001 From: Juho Teperi Date: Fri, 20 Sep 2019 11:03:19 +0300 Subject: [PATCH 3/3] Implement trailing slash handler as custom Router --- .../reitit-frontend/src/reitit/frontend.cljs | 41 +++++++++++----- test/cljs/reitit/frontend/core_test.cljs | 47 ++++++++++--------- 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/modules/reitit-frontend/src/reitit/frontend.cljs b/modules/reitit-frontend/src/reitit/frontend.cljs index dadbe54ca..75eb2d6d1 100644 --- a/modules/reitit-frontend/src/reitit/frontend.cljs +++ b/modules/reitit-frontend/src/reitit/frontend.cljs @@ -15,21 +15,37 @@ (map (juxt keyword #(.get q %))) (into {})))) +(defn trailing-slash-router [parent method] + (if method + ^{:type ::r/router} + (reify r/Router + (router-name [_] + :trailing-slash-handler) + (routes [_] + (r/routes parent)) + (compiled-routes [_] + (r/compiled-routes parent)) + (options [_] + (r/options parent)) + (route-names [_] + (r/route-names parent)) + (match-by-path [_ path] + (or (r/match-by-path parent path) + (if (str/ends-with? path "/") + (if (not= method :add) + (r/match-by-path parent (subs path 0 (dec (count path))))) + (if (not= method :remove) + (r/match-by-path parent (str path "/")))))) + (match-by-name [_ name] + (r/match-by-name parent name))) + parent)) + (defn match-by-path "Given routing tree and current path, return match with possibly coerced parameters. Returns nil if no match found." [router path] - (let [uri (.parse Uri path) - path (.getPath uri)] - (if-let [match (or (r/match-by-path router path) - (if-let [trailing-slash-handling (:trailing-slash-handling (r/options router))] - ;; TODO: Maybe the original path should be added under some key in match, - ;; so it is easy for user to see if trailing slash "redirect" happened? - (if (str/ends-with? path "/") - (if (not= trailing-slash-handling :add) - (r/match-by-path router (subs path 0 (dec (count path))))) - (if (not= trailing-slash-handling :remove) - (r/match-by-path router (str path "/"))))))] + (let [uri (.parse Uri path)] + (if-let [match (r/match-by-path router (.getPath uri))] ;; User can update browser location in on-navigate call using replace-state (let [q (query-params uri) match (assoc match :query-params q) @@ -62,7 +78,8 @@ ([raw-routes] (router raw-routes {})) ([raw-routes opts] - (r/router raw-routes (merge {:compile rc/compile-request-coercers} opts)))) + (-> (r/router raw-routes (merge {:compile rc/compile-request-coercers} opts)) + (trailing-slash-router (:trailing-slash-handling opts))))) (defn match-by-name! "Logs problems using console.warn" diff --git a/test/cljs/reitit/frontend/core_test.cljs b/test/cljs/reitit/frontend/core_test.cljs index 1a56934ba..ffeccf317 100644 --- a/test/cljs/reitit/frontend/core_test.cljs +++ b/test/cljs/reitit/frontend/core_test.cljs @@ -12,10 +12,11 @@ (deftest match-by-path-test (testing "simple" - (let [router (r/router ["/" - ["" ::frontpage] - ["foo" ::foo] - ["bar" ::bar]])] + (let [router (rf/router + ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar" ::bar]])] (is (= (r/map->Match {:template "/" :data {:name ::frontpage} @@ -51,10 +52,11 @@ (rf/match-by-name! router ::asd))))))))) (testing "schema coercion" - (let [router (r/router ["/" - [":id" {:name ::foo - :parameters {:path {:id s/Int} - :query {(s/optional-key :mode) s/Keyword}}}]] + (let [router (rf/router + ["/" + [":id" {:name ::foo + :parameters {:path {:id s/Int} + :query {(s/optional-key :mode) s/Keyword}}}]] {:compile rc/compile-request-coercers :data {:coercion rsc/coercion}})] @@ -108,11 +110,12 @@ (deftest trailing-slash-handling-test (testing ":both" - (let [router (r/router ["/" - ["" ::frontpage] - ["foo" ::foo] - ["bar/" ::bar]] - {:trailing-slash-handling :both})] + (let [router (rf/router + ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] + {:trailing-slash-handling :both})] (is (= (r/map->Match {:template "/foo" :data {:name ::foo} @@ -136,10 +139,11 @@ (rf/match-by-path router "/bar"))) )) (testing ":add" - (let [router (r/router ["/" - ["" ::frontpage] - ["foo" ::foo] - ["bar/" ::bar]] + (let [router (rf/router + ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] {:trailing-slash-handling :add})] (is (= (r/map->Match {:template "/foo" @@ -164,10 +168,11 @@ (rf/match-by-path router "/bar"))))) (testing ":remove" - (let [router (r/router ["/" - ["" ::frontpage] - ["foo" ::foo] - ["bar/" ::bar]] + (let [router (rf/router + ["/" + ["" ::frontpage] + ["foo" ::foo] + ["bar/" ::bar]] {:trailing-slash-handling :remove})] (is (= (r/map->Match {:template "/foo"