Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: made trailing slash optional for namespaced routes #42

Merged
merged 1 commit into from
Apr 16, 2018

Conversation

ulken
Copy link
Contributor

@ulken ulken commented Mar 13, 2018

When using withNamespace(), requesting the "index" route of the namespace requires a trailing slash to match. The namespace example at the bottom of the readme does not work without appending a trailing slash to the url.

Example code:

const apiRoute = withNamespace('/api')

const routes = router(
  apiRoute(get('/', () => 'My api route'))
)

// Start server...

request(`${SERVER_URL}/api`)  <-- Does NOT work
request(`${SERVER_URL}/api/`) <-- Works

This PR makes the trailing slash optional for namespaced routes (in par with non-namespaced index routes). Note: It does not make trailing slashes optional for all routes, only for index routes.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 30d562b on ulken:master into f959b7f on pedronauck:master.

@ulken
Copy link
Contributor Author

ulken commented Mar 13, 2018

An alternative approach would be to change the default to only match without trailing slash. If someone wants to be strict and require trailing slash they can always use a custom UrlPattern. Makes more sense to me at least.

Some might be concerned about SEO and duplicate content leaving it optional. Just putting it out there.

@pedronauck pedronauck merged commit 40e478c into pedronauck:master Apr 16, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants