Change to ranking and sorting #136

ashsearle · 2017-02-01T17:03:12Z

These changes address issues #123 and go some way to help with #63, and update bits of code to use more built-in array methods.

The ranking proposed here works on a super-set principle, with each segment of the pathname ranked in turn. From highest rank to lowest: /foo, /:foo, /:foo?, /:foo+, /:foo*

The rank function now produces a string, and uses string sorting to rank. This allows comparison at each level of a path:
/a/:b/:c? would be ranked as 543 which (using string comparison) is higher ranked than /a (5), but lower than /a/b (55)

developit · 2017-02-01T17:33:55Z

Hi @ashsearle - very happy to see action on this! Only thing I have to ask - the Array changes are actually problematic. This library supports IE9+ (all Preact libs do) and things like [].some() don't exist there. They also transpile to a fair bit more code. Any chance we could split the Array stuff into a separate PR from the ranking?

developit · 2017-02-01T17:35:49Z

src/index.js

@@ -84,7 +75,7 @@ function routeFromLink(node) {
 		target = node.getAttribute('target');

 	// ignore links with targets and non-path URLs
-	if (!href || !href.match(/^\//g) || (target && !target.match(/^_?self$/i))) return;
+	if (!/^\//.test(href) || (target && !target.match(/^_?self$/i))) return;


Good call. While we're here, might be worth extending this check to also ignore javascript: prefixed href values.

That should already be handled as "javascript:" doesn't start with "/"

Man I need more coffee today eh

developit · 2017-02-01T17:36:41Z

src/index.js

+				if (matches) {
+					if (invoke!==false) {
+						// copy matches onto props
+						Object.assign(attributes, { url, matches }, matches);


I don't think Object.assign is transpiled in this repo

It was already in use in the Link function (but think that might have been a recent change: in the last few months or so...)

const Link = (props) => { return h('a', Object.assign({}, props, { onClick: handleLinkClick })); };

Ah you're right, and it is being transpiled. My bad!

developit · 2017-02-01T17:39:17Z

src/util.js

+		const queryParams = {};
+		search.split('&').forEach(parameter => {
+			let [name, ...value] = parameter.split('=');
+			queryParams[decodeURIComponent(name)] = decodeURIComponent(value.join('='));


Thought: instead of the object rest spreads, maybe assign directly into matches here and skip queryParams entirely?

I wanted to avoid wasted effort parsing query parameters on routes that didn't match, but didn't want to change behaviour.

e.g. with a route of /:a and url of /foo?a=bar, you either end up with matches being {a: 'foo'} (path takes priority - current behaviour in 2.4.1) or {a: 'bar'}(query param takes priority, which would happen if I assigned direct to matches after moving code down here.)

@ashsearle makes sense. What about just collecting up the query params under $query?

I'm not sure if you're just suggesting renaming queryParams or something else(?)

Thinking about this a bit more: you could do a bit of refactoring and extract the url parsing from exec. You could go as far as splitting to pathname and url, and running segmentize(pathname) before calling exec. Then, in terms of $query parsing / handling, that could be done in the if (invoke!==false) block in getMatchingChildren...

I had meant instead of merging queryParams into matches, do matches.$query = queryParams to separate the two. Not sure which is better.
Agreed about pulling out the query param parsing, exec should never need to look at the querystring anyway, so that seems like a good call. I'm impartial to whether we call segmentize() before exec() or within it - probably nice to do the split once and re-use, but only if its not annoying.

developit · 2017-02-01T17:40:02Z

src/util.js

-	return diff || (aAttr.path.length - bAttr.path.length);
+	let aRank = rank(aAttr.path),
+		bRank = rank(bAttr.path);
+	return (aRank < bRank) ? 1 :


seems like subtraction could work here

It won't work with subtraction with string ranks: I need '5' < '543' < '55'.

Using < and > will work for both numeric and string ranks - so gives that little extra flexibility if you need to try out / plug-in different ranking methods later.

Yeah, sorry I commented here before I re-read your technique and understood the idea. It works nicely!

developit · 2017-02-01T17:48:37Z

src/util.js

-	return (strip(url).match(/\/+/g) || '').length;
+export function rank(path) {
+	return strip(path).
+	        replace(/(:)?([^\/]*?)([*+?])?(?:\/+|$)/g, (match, isParam, segment, flag) => {


I like this scoring. Would prefer an exec-based tokenizer though to keep the math numeric.

Just spitballing, but here's what I came up with:

path = strip(path); let rank=0, token, reg = /(:)?([^\/]*?)([*+?])?(?:\/+|$)/g; let flags = ['*', '+', '?']; while (token=reg.exec(path)) { let [, isParam, segment, flag] = token; rank += 100 + (segment ? 5 : isParam ? (flags.indexOf(flag)+1 || 4) : 0); } return rank;

I just re-read your comment about the ranking and take back the numeric bit. Still nice to have the exec()-based rather than replace() based parse just for perf.

I found the exec hard a little gnarly to work with, and went through a few different sort implementations. The thing I find weird about exec is it's doing double-duty: doing the equivalent of 'exec' and 'test' at the same time (stealing terminology from regex.)

In my defence: the ranking is done via a Schwartzian transform before sorting, so it's only done once per route, not multiple times as you might expect in a dumb sort.

I was mostly thinking of avoiding the function invocation and replace since it's mutative. That said, since the output is actually being used I'm more amenable to it (now that I properly understand the sort ordering approach).
Only thing I'd change, then, would be to pull out the replace callback and make it a named function. It's pure, so there's no need to recreate it on each call to rank() 👍

ashsearle · 2017-02-01T18:45:20Z

Hi - it's nice to have such a quick response.

The array methods used should all be fine in IE9. I checked your browser targets against info at MDN which says that some, and reduce (and every) are supported in IE9, along with the old stalwarts of sort, map and filter.

Just checked MSDN too: some and reduce are supported in IE9.

I did think it was odd that Object.assign was being used though: I'm happy to remove that if you want...?

ashsearle · 2017-02-05T11:20:21Z

I've made rank() more compact (closer to your spitball approach), and use a named function for callback to avoid recreating on each invocation. I think it's a bit more readable now too.

I tried pulling out a separate $query property on matches for query-parameters as suggested, but this throws a DOMException on this line:

Object.assign(attributes { url, matches }, matches);

This is basic DOM restriction that you can't have a property called $query hanging on a DOM element (try reproducing with document.body.setAttribute('$query', 'anything')) I don't have a good suggestion for how else to handle this at the moment, so have left code as is: preserving existing query-parameter handling behaviour.

developit · 2017-04-17T23:32:39Z

Not 100% I can safely resolve conflicts here. Maybe we switch back to the single sort? I can't remember if that was something related to this PR.

ashsearle added 4 commits January 28, 2017 16:12

Use classic array methods, and more es2015

854cc48

Merge remote-tracking branch 'upstream/master'

966a658

Implement stable sort to fix #123

9822495

Merge remote-tracking branch 'upstream/master'

cbeeeeb

developit reviewed Feb 1, 2017

View reviewed changes

ashsearle added 2 commits February 5, 2017 10:59

Use named function for rank callback

ffd8cc5

Ignore text children of Router

3cf318c

Merge remote-tracking branch 'upstream/master'

add0f95

ashsearle mentioned this pull request Feb 24, 2017

Handle relative URLs #138

Closed

developit mentioned this pull request Aug 23, 2017

Feature: accept regular expression matching #227

Open

ashsearle closed this Oct 22, 2017

ashsearle mentioned this pull request Oct 22, 2017

Fix #123: Implement new path ranking algorithm #242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change to ranking and sorting #136

Change to ranking and sorting #136

ashsearle commented Feb 1, 2017

developit commented Feb 1, 2017

developit Feb 1, 2017

ashsearle Feb 1, 2017

developit Feb 1, 2017

developit Feb 1, 2017

ashsearle Feb 1, 2017

developit Feb 1, 2017

developit Feb 1, 2017

ashsearle Feb 1, 2017

developit Feb 1, 2017

ashsearle Feb 1, 2017 •

edited

Loading

developit Feb 2, 2017 •

edited

Loading

developit Feb 1, 2017

ashsearle Feb 1, 2017

developit Feb 1, 2017

developit Feb 1, 2017

developit Feb 1, 2017

ashsearle Feb 1, 2017 •

edited

Loading

developit Feb 1, 2017 •

edited

Loading

ashsearle commented Feb 1, 2017 •

edited

Loading

ashsearle commented Feb 5, 2017

developit commented Apr 17, 2017

Change to ranking and sorting #136

Change to ranking and sorting #136

Conversation

ashsearle commented Feb 1, 2017

developit commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashsearle Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

developit Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashsearle Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

developit Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

ashsearle commented Feb 1, 2017 • edited Loading

ashsearle commented Feb 5, 2017

developit commented Apr 17, 2017

ashsearle Feb 1, 2017 •

edited

Loading

developit Feb 2, 2017 •

edited

Loading

ashsearle Feb 1, 2017 •

edited

Loading

developit Feb 1, 2017 •

edited

Loading

ashsearle commented Feb 1, 2017 •

edited

Loading