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

Implement #238, handle trailing slashes in frontend match-by-path #239

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Deraen
Copy link
Member

@Deraen Deraen commented Mar 15, 2019

Does adding option to router make sense? Are there other ways to control this logic?

(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)
Copy link
Member

Choose a reason for hiding this comment

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

This will make the lookup much slower as there is an extra hash-lookup for each routing call. Would be better to move the check into router creation time and return a reified r.c/Router with different code for the different cases.

Copy link
Member

Choose a reason for hiding this comment

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

Still think this would be cleaner to be implemented as a reified Proxy, but that could be done later too.


| key | description |
| -------------|-------------|
| :trailing-slash-handling | TODO |
Copy link
Member

Choose a reason for hiding this comment

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

could be just :trailing-slash (or :trailing-slash-method (the ring-version has :method)).

Copy link
Member

Choose a reason for hiding this comment

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

@Deraen Deraen force-pushed the feature/frontend-trailing-slash-handling branch from 0923527 to 54a26ad Compare September 20, 2019 07:45
@Deraen
Copy link
Member Author

Deraen commented Sep 20, 2019

Now implemented as custom Router, which wraps the parent router.

TODO:

  • Move to reitit-core
  • Migrate reitit-ring to use this.
  • Decide on API, do we add option on some basic routers? Maybe even reitit.core/router, or should user wrap another router in trailing-slash-router?
  • Check performance, alternative implementation would be to modify routes to match additional / missing slashes in Router creation time.

@Deraen
Copy link
Member Author

Deraen commented Sep 20, 2019

I remembered some reason why it might not be necessary or even good idea to move this to core:

Frontend and backend have quite a different solutions on what to do in these cases: this frontend solution just returns the match as if the original route was matched. In backend ring handler will return redirect response.

@orestis
Copy link

orestis commented Nov 13, 2019

I can't talk about the implementation details (moving to core or not), but my 2c on (my) desired behaviour in the frontend:

  • Generally speaking, would like to have a "canonical" representation of the route. It's probably application specific about whether this is with, or without a trailing slash. This is something we can control today by using "" or "/" in a route definition.
  • Any non-canonical but equivalent paths are coming from either users typing directly into the URL bar (e.g. stripping off a path segment) or external applications linking in (where someone types the URL into an input field). In both cases, we can't control what the link looks like.
  • Would like the frontend to silently accept the equivalent path, and route to the canonical one. Sometimes we might want to change the visible URL to reflect the canonical path, but sometimes that's not necessary.

Thank you for your work on this!

@orestis
Copy link

orestis commented Nov 13, 2019

Not sure if useful for anyone, but here's a small snippet that seems to do the trick:

route! and navigation-fail are application-specific handlers. The try-without-slash is run upon a match fail, to see if we can recover.

(defn- try-without-slash [path router]
  (if (str/ends-with? path "/")
    (let [non-slash-path (subs path 0 (dec (count path)))]
      (if-let [match (rf/match-by-path router non-slash-path)]
        (do
          (.replaceState js/window.history nil "" non-slash-path)
          (route! match))
        (navigation-fail non-slash-path)))
    (navigation-fail path)))

(if (not= method :remove)
(r/match-by-path parent (str path "/"))))))
(match-by-name [_ name]
(r/match-by-name parent name)))
Copy link

Choose a reason for hiding this comment

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

also need implementation

(match-by-name [_ name path-params]
        (r/match-by-name parent name path-params))

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.

4 participants