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

Allowing option hashes for #225 #230

Closed
wants to merge 7 commits into from
Closed

Allowing option hashes for #225 #230

wants to merge 7 commits into from

Conversation

mehcode
Copy link
Member

@mehcode mehcode commented Oct 12, 2012

Allows for say:

@publishEvent '!router:route', '/some/url/here/', replace: true

Still trying to wrap my head around expect.js, etc to add tests for it -- any good pointers in the right direction?

This is for #225

@paulmillr
Copy link
Contributor

Tests would be nice. Just change spec/coffee/router spec etc.

@mehcode
Copy link
Member Author

mehcode commented Oct 15, 2012

Added tests for !router:route and !router:changeURL. Not sure how to test that when !router:route is invoked with options !router:changeURL should be invoked with the same options. Any ideas? Still learning this expect.js / mocha / etc thing.

Can't figure out how to assert that the history is being populated (and not populated) either.

@paulmillr
Copy link
Contributor

Not sure how to test that when !router:route is invoked with options !router:changeURL should be invoked with the same options

Just subscribe to !router:changeURL and check for identity (is) of objects. Then unsubscribe after compare.

Can't figure out how to assert that the history is being populated (and not populated) either.

try window.history.back() or so.

@mehcode
Copy link
Member Author

mehcode commented Oct 16, 2012

Added the rest of the tests. Test suite covers options being passed in from several points all the way until chaplin hands them off to backbone.

@molily
Copy link
Member

molily commented Oct 17, 2012

This is a good idea, thanks for the work.

I’m thinking that we could use this change to clean up a bit internally. At the moment, the params hash is (mis)used for several things. It contains query string parameters, URL pattern matches and fixed route parameters. But there are additional magic properties like changeURL, forceStartup and path. Chaplin uses them internally and they should better be separated. params should be the pure URL input for the controller, no internal routing information.

With your changes there’s a second options hash which is used for Backbone.history.navigate but also might have a changeURL property AFAICS. I’m wondering if we could use this hash for all Chaplin routing information in order to separate it from the params. Any ideas on this?

@mehcode
Copy link
Member Author

mehcode commented Oct 19, 2012

Definitely agree with moving non parameters out of the params hash.

I moved everything I could find that wasn't directly related to the params-hash out of params and into the options hash. I then removed things that backbone wasn't aware of before sending it off to backbone.

Let me know if I missed anything, @molily

@molily
Copy link
Member

molily commented Oct 25, 2012

Cool, I’ll have a closer look soon. Thanks!

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

3 participants