CodeYellowBV / tarantino

a tiny and isomorphic URL router for JavaScript.
MIT License
11 stars 3 forks source link

break up setRoute into separate methods? #11

Open albertalquisola opened 7 years ago

albertalquisola commented 7 years ago

This is more of a usability enhancement vs. new feature but what do you think about breaking up the setRoute function into separate methods? I dislike how 'overloaded' it is. Depending on the params you pass in, the function will perform slightly different actions, making it hard to reason about and use.

RIght now, setRoute can be used in 4 different ways:

  1. setRoute(route)

    • navigate to new route
  2. setRoute(start, length)

    • remove a segment from the current route
  3. setRoute(index, value)

    • replace a segment of the current route
  4. setRoute(start, length, value)

    • remove and set a segment on the current route

Caveat is that this will introduce breaking changes to the router (I guess we could make it backwards compatible but not a huge fan of that idea)

Also, I'm willing to submit a PR for this if you think it's a good idea

Thoughts?

albertalquisola commented 7 years ago

went ahead and wrote some of the new potential APIs to get a better sense of how theyd look:

Router.prototype.removeSegment = function(options) {
  var url = this.explode();

  if (start < url.length) {
    url.splice(options.start, options.length);
  }

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.replaceSegment = function(options) {
  var url = this.explode();
  url[options.index] = options.value;

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.removeAndSetSegment = function(options) {
  var url = this.explode();

  if (options.start < url.length) {
    url.splice(options.start, options.length, options.value);
  }

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.setRoute = function(options) {
  var url = [options.route].join('/');
  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.replaceRoute = function(options) {
  var url = [options.route];
  listener.replaceHash(url.join('/'), options.state);

  return url;
};
mhulse commented 7 years ago

Sounds like a good idea, and looks much more readable to me. Why don't you do a PR, with updated tests, so others can do functional reviews? Thanks @AlbertAlquisola!!!!