Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Fix matching routes with trailing slash (refs #532) #727

Merged
merged 1 commit into from
Dec 13, 2013

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Dec 6, 2013

Hi,

It's an attempt to fix non-functioning routes when using trailing slash. I stumbled upon it while testing my app with urls like http://app-domain.dev/recipes vs http://app-domain.dev/recipes/, where in the latter case Chaplin doesn't recognise my routes anymore.

My routes.js.coffee:

define ->
  'use strict'

  (match) ->

    # ... snip ...

    match 'recipes',          'recipes#index', name: 'recipes'
    match 'recipes/new',      'recipes#new',   name: 'new_recipe'
    match 'recipes/:id',      'recipes#show',  name: 'recipe'
    match 'recipes/:id/edit', 'recipes#edit',  name: 'edit_recipe'

    # ... snip ...

@akre54
Copy link
Contributor

akre54 commented Dec 6, 2013

Relevant backbone discussion: jashkenas/backbone#848

@paulmillr
Copy link
Contributor

I think we should instead redirect users from trailing-slash-URLs to non-trailing-slash-URLs (can be disabled in config).

What do you think?

users/paulmillr/ => users/paulmillr

No mess with SEO.

@Sija
Copy link
Contributor Author

Sija commented Dec 7, 2013

👍

@paulmillr
Copy link
Contributor

Can you also please handle null case with true / false? On null it should behave as it currently behaves in 0.12

@@ -77,6 +77,13 @@ module.exports = class Route
value = params[name]
url = url.replace ///[:*]#{name}///g, value

# Add or remove trailing slash according to options
switch @options.trailing
Copy link
Contributor

Choose a reason for hiding this comment

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

How about if @options.trailing isnt false? A switch on a boolean seems a bit weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, going further if @options.trailing is true is even simpler ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

But we want null/undefined to set trailing to true also. The only time we don't want it true is when trailing is false, hence trailing isnt false :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the referenced issue #532 setting values other than true or false would perform no redirecting whatsoever. That would be also my semantic understanding (yes–add a slash, no—remove it, null/undefined—abstain from action).

@Sija
Copy link
Contributor Author

Sija commented Dec 12, 2013

@paulmillr You mean in the tests? Right now there's a default setting trailing: no which makes Chaplin behave in a new but BC-compatible way.

@paulmillr
Copy link
Contributor

I'll check it!

@paulmillr
Copy link
Contributor

Awesome. Can you please squash all commits to one and i'll merge this?

Implementation is consistent with the one proposed in chaplinjs#532
@Sija
Copy link
Contributor Author

Sija commented Dec 13, 2013

Voila! :)

paulmillr added a commit that referenced this pull request Dec 13, 2013
Fix matching routes with trailing slash (refs #532)
@paulmillr paulmillr merged commit d95bb31 into chaplinjs:master Dec 13, 2013
@paulmillr
Copy link
Contributor

Thanks dude — this is awesome

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants