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

Split params into params and routing options #248

Merged
merged 23 commits into from Nov 28, 2012
Merged

Conversation

molily
Copy link
Member

@molily molily commented Nov 5, 2012

#230 with several changes:

  • Perform the options check where the navigate call is located (Router#changeURL), not in the Dispatcher
  • Router#routeHandler: Don’t be backwards compatible
  • Moved path also to the routing options to allow for a path param. Does this make sense?
  • Improved the spec using a stub
  • Updated the testing library stack and switched to RequireJS 2.1 which always performs async module loading
  • Several style changes on the specs.

BEWARE, this will introduce a API-breaking changes. The new signatures are:

  • !router:route: path, options, callback
    • This is probably the most breaking change
  • matchRoute: route, params, options
  • !startupController: controllerName, action, params, options
  • Controller#redirectTo: path or (controllerName, action, params, options)

Flags that control the routing (like forceStartup, changeURL, trigger/replace) and information from the router (path) have to be added to the new options hash. Formerly, they were part of the params hash which could introduce conflicts with real URL or query string parameters.

TO BE DISCUSSED:

I’m not sure if it makes sense to make this change in a backwards-compatible way. This would introduce a lot of ugly code. We never claimed that Chaplin’s API is stable and I think we should make this cut as early as possible. It’s a bit cumbersome to have a second hash, but internal routing information like path shouldn’t be part of params before the routing/controllers API is getting stable.

@paulmillr
Copy link
Contributor

should listen to the !router:route even test fails for me (chrome 22 stable, running w/o a webserver).

@molily
Copy link
Member Author

molily commented Nov 6, 2012

This needs more testing anyhow since I just discovered a problem that wasn’t caught by the specs.

Test that redirectTo throws an exception
Test for routing options on !router:route
Test for protocol-relative URLs
Style changes
Test routing options more properly
Improve check if Backbone history was stopped
Remove duplicate test, merge into one
Renamed variables for consistency
@molily
Copy link
Member Author

molily commented Nov 22, 2012

Okay, now it should be ready for prime time.

@molily
Copy link
Member Author

molily commented Nov 22, 2012

Maybe it’s a good idea to support the old !router:route signature like @mehcode did since this is quite simple. (I had removed the code but now I’m unsure.)
Still this will introduce some breaking changes, for example params.path will be options.path. To support these old cases will be a lot more complex and lead to ugly code.

@paulmillr
Copy link
Contributor

Looks good for me. Will merge in a bit, after testing locally. Backwards-compat is okay here.

@paulmillr paulmillr closed this Nov 27, 2012
@paulmillr paulmillr reopened this Nov 27, 2012
@paulmillr
Copy link
Contributor

the travis build still fails

paulmillr added a commit that referenced this pull request Nov 28, 2012
Split params into params and routing options
@paulmillr paulmillr merged commit 24b3527 into master Nov 28, 2012
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.

None yet

2 participants