Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

should crossroads be case insensitive? #53

Closed
millermedeiros opened this Issue · 7 comments

2 participants

@millermedeiros

the issue #49 made me think about it:

  1. should routes also be case insensitive? so the route foo/bar would be the same as Foo/BAR.
  2. should crossroads always pass values as lowercase to listeners? (this might cause issues with the query string feature and may be undesirable)

PS: for now I will implement it as a branch but not merge it into master..

@ducka

Wow, I just learnt something new. Apparently URLs can be case sensitive:

http://www.w3.org/TR/WD-html40-970708/htmlweb.html

After reading that, I would suggest leaving case sensitive matching in crossroads as a default, but add the ability to disable it if you so require. I've never written a web app that's required case sensitive URLs before, so having case sensitive matching of routes and whatnot in crossroads just trips me up; I'd imagine however, that you would want to code as close to the W3C URL spec as possible to cover your bases.

@millermedeiros

@ducka what you can do for now is to make sure you always call String#toLowerCase() before calling crossroads.parse() and always write your routes in lowercase. I'll keep developing the ignorecase branch, just not sure when/if it will ship (maybe it will increase complexity a lot for no real gain).

@ducka

No worries @millermedeiros. Thanks for looking into this.

@millermedeiros

I added the option to toggle case sensitivity on the branch ignorecase, it didn't increased complexity that much. Check comparison: v0.9.0...ignorecase - I also reverted the behavior to be case sensitive by default to match old behavior and also because of performance (case sensitive will be theoretically faster while using array validation rules since it doesn't need to convert items to lowercase - shouldn't really matter in real apps since arrays will probably contain few items).

I'm considering to merge this into the dev branch and release on the next version since it increases flexibility... but I'm becoming a little bit concerned about extra features since I feel crossroads is becoming too complex (I'm only using half of the features on my own apps and kinda regret of adding features like "greedy" routes).

@ducka do you think it pays-off the extra lines of code? Do you still think it's an useful feature?

Thanks!

@ducka

Heya @millermedeiros, sorry for the late response to this. As a .Net developer working prominently with windows based systems and IIS, I really don't have any situation where case sensitivity is a requirement for the web applications I write. I think having this sort of functionality baked into the library would be a must have, so I'd be for merging this into your main dev branch.

@millermedeiros millermedeiros closed this issue from a commit
@millermedeiros Merge branch 'ignorecase' into dev and change ignoreCase to true by d…
…efault. closes #53. closes #49.

* ignorecase:
  fix typo on changelog
  set ignoreCase to false by default. see #53
  add option to toggle case sensitivity. see #53
  made route matching case insensitive. see #53
  changed `Route.rules` array validation to be case insensitive. see #49

Conflicts:
	CHANGELOG.md
	dev/src/crossroads.js
	dev/src/route.js
	dev/tests/spec/match.spec.js
40bb0c8
@millermedeiros

Decided to set ignoreCase to true by default since perf shouldn't be an issue in 99.99% of the cases and it will probably avoid more issues than it will cause.

I plan to ship v0.11.0 soon, check changes here: v0.10.0...v0.11.0a

Thanks for all the feedback and sorry for the delay on merging this one. Cheers.

@ducka

No worries Miller, thanks a bunch for this awesome library :D

@millermedeiros millermedeiros referenced this issue from a commit
@millermedeiros Merge branch 'dev'
* dev:
  add ignoreState. closes #57
  add test for resetState()
  bump version to 0.11.0a and do a build
  add crossroads.pipe() and crossroads.unpipe(). closes #70
  refactor match and parse specs to avoid redundant `shouldTypecast` reset, update changelog.
  Query strings should respect the shouldTypecast property.
  fix typo on changelog
  set ignoreCase to false by default. see #53
  add option to toggle case sensitivity. see #53
  made route matching case insensitive. see #53
  changed `Route.rules` array validation to be case insensitive. see #49
045ac26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.