Kill `null` arg sent to all route handlers

[beck]

A side-effect of parsing the optional query arg.

Discovered when trying to do something like:

var _ = require('lodash');
var greet = require('./greet');

Router.extend({

  routes: {
    ':name/:greeting': 'greet',
    'hi-doug': 'hiDoug',
    'hi/:name': 'hiPerson',
  },

  greet: greet,
  hiDoug: _.partial(greet, 'doug', 'hi'),
  hiPerson: _.partialRight(greet, 'hi'), // fails via `greet('NameFromUrl', null, 'hi')`
});

[dhritzkiv]

At first glance, this looks good to me. I'm gonna run through the tests and logic this weekend. It'll probably be a SEMVER major change by the looks of it (which is nbd)

[dhritzkiv]

I agree with these proposed changes. Tests pass locally. Going to let another member or two +1 this.

[wraithgar]

Yes this is a good update and I agree it's a major semver. Thank you for taking the time to submit this @beck

[dhritzkiv]

Released in v5.0.0

[cquinders]

I was using the optional (but always present) query string argument in the execute method to always parse the query params and pass them to the route callback as an object.

With this change I don’t see a viable solution to this anymore, because it is hard to detect if the last arg is a named param or the optional query string.

I liked the fact, that the last arg always contained the value of the query string (and therefore null if no query string was present). To me this was a more reliable solution to handling query strings.

[dhritzkiv]

What happens now when the query string is present and what happens when the query string is not present?

[cquinders]

When a query string is present, it is available in the last arg. If no query string is present, the last argument is the last named param from the route.

Consider this Router for example:

const MyRouter = Router.extend({

  routes: {
    ':categoryId': 'category'
  },

  category(...args) {
    console.log(args);
  }

});

In ampersand-router@4

MyRouter.navigate("/books");
// ["books", null]
MyRouter.navigate("/books?page=2");
// ["books", "page=2"]

In ampersand-router@5

MyRouter.navigate("/books");
// ["books"]
MyRouter.navigate("/books?page=2");
// ["books", "page=2"]

Differentiating between params and query string is hard now, because they are all strings.

[beck]

@cdaringe is it possible to use route handlers that are more expressive with the accepted arguments?

categrory(categoryId, query)

Would work, with @5 query will now be undefined instead of null.

[cquinders]

@beck For most people this would be a viable solution, but they would have to do the parsing of query params in multiple route handlers now, because this example from the AmpersandRouter#execute docs is not working anymore:

var Router = AmpersandRouter.extend({
  execute: function(callback, args) {
    args.push(parseQueryString(args.pop()));
    if (callback) callback.apply(this, args);
  }
});

I’m not able to use this work around because I extended AmpersandRouter with some additional functionality that hooks into execute. Currently this is no big deal fro me. I’ll just stick to ampersand-router@4 by now and maybe do a fork if the need arises for me.

I just wanted to share my perspective on this in case someone else will have issues with this change in the future.

[cdaringe]

hey @beck, truth be told, in &js-land, i never used router practically in prod. we always shimmed it into existing apps w/ routing infrastructure in place. i regret to inform that i've checked out of active &js maintenance for the most part