kadirahq / flow-router

Carefully Designed Client Side Router for Meteor
MIT License
1.09k stars 194 forks source link

Add '|' and '-' in regex fixes #479 #499

Closed tomwasd closed 8 years ago

tomwasd commented 8 years ago

As reported in #479 flow router wasn't working with certain regex in the path. This PR allows for path definitions such as:

/blog/:action(edit|create)

Added the hyphen to also allow for:

/blog/:action(edit-page|create-page)

There might be some other URL-friendly characters that need to be catered for but this should help in most cases!

arunoda commented 8 years ago

There is a new feature of path-to-regexp which does this. At the time I'm writing this it was not there. Could you simply use that in this PR: https://github.com/pillarjs/path-to-regexp#compile-reverse-path-to-regexp

tomwasd commented 8 years ago

At first glance looks like pathToRegex.compile handles things differently to the custom code before?

E.g.

var pathDef = '/blog/:blogId/some/:name';
  var fields = {
   blogId: '1001',
};
var expectedPath = '/blog/1001/some';`

But with pathToRegex.compile an exception is thrown rather than a path returned due to :name being missing.

Not sure how you want to handle it as if people have relied on that functionality I guess it could constitute a breaking change?

arunoda commented 8 years ago

That's because name is not an optional parameter. If path def looks like this, I think it's fine.

var pathDef = '/blog/:blogId/some/:name';

We are planning for version 4.0 and it's a pretty good time for a breaking change.

Is there any other issue other than this?

tomwasd commented 8 years ago

Looks like pathToRegex encodes the URL params too which was causing some of the other tests to fail.

I've updated all the tests to pass although I'm still a little new when it comes to testing so you might want to give it a once over!

arunoda commented 8 years ago

Great. I know what's happening there. See here: https://github.com/kadirahq/flow-router/blob/ssr/client/router.js#L160

I encoded in the client side. So, here are the changes we need.

This will work properly then.

arunoda commented 8 years ago

Awesome. This is great.

jamesloper commented 8 years ago

Hey man love your work, big fan.

Just letting you know I tracked down a bug that basically disallowed you from using [] in route definitions. Usage caused FlowRouter#go to put some URL encoded characters in the url.

The following regExp was altered in Router.prototype.path

var regExp = /(:[\w\(\)\\\+\*\.\?\{\}\[\]-]+)+/g;