sammyt / rhumb

URL routing in js
MIT License
4 stars 4 forks source link

Handle simple query string parameters #3

Closed aaronhaines closed 9 years ago

aaronhaines commented 10 years ago

Initial query string handling.

Next should add roll up of duplicate params in to lists.

sammyt commented 9 years ago

roll up of duplicate params in to lists.

Makes a lot of sense

mstade commented 9 years ago

Yup :+1: for sure. I'll merge when #6 is done.

mstade commented 9 years ago

I think I understand this PR, but I'd like some verification. Does this change make it so that any query param – regardless of whether the template describes them – is added to the params object passed to the handler function? If I read the code correctly, it would also make sure that any query param added, won't override a path parameter. For example: when matching /sing/bird-song?sound=bark /sing/{sound} the resulting params object would be { sound: 'bird-song' }. Is that the correct and expected behavior?

mstade commented 9 years ago

By the way, I added a test to verify my expectations of the path vs. query param order, and it did indeed favor path params when there's a conflict. I think this behavior makes sense given how things work at the moment, but on the other hand it may also be cause for discussing whether different parameters should actually be returned in different fields.

I can't off hand come up with a good scenario where you'd have path parameters and query parameters be named the same, but I also can't quite come up with a reason why it shouldn't be allowed. It's probably a discussion to be held outside of this PR, but the original question remains – is this behavior reasonable and expected?

mstade commented 9 years ago

For the record, here's the test:

it("should not overwrite path params with query params", function() {
  var router = rhumb.create()

  router.add("/sing/{sound}", function(params) {
    return params
  })

  expect(router.match("/sing/bird-song?sound=bark")).to.eql({
    sound: "bird-song"
  })
})

To run the tests, make sure you've rebased or merged master into this branch, then do either npm test or npm run test-browser depending on whether you want to run the tests in a terminal or a browser.

mstade commented 9 years ago

As per yesterday's discussion, I'm merging this and opening up new issues for the remaining work.