Add support for query parameters

[nahtnam]

Currently, if you have a URL such as /?hello=world, the router says that the route is not found. I made changes so that it ignores all query params and matches only the route itself (/).

[coveralls]

Coverage remained the same at 100.0% when pulling 0497028 on ludicrousxyz:master into 36a06a5 on protocol114:master.

[nickduncan7]

I like the idea, but if I recall correctly the url.parse method was deprecated in Node 11.0.0. I'll take your PR as a starting point, write a test against this case and implement this change with the newer WHATWG URL API. Thanks!

[nahtnam]

Great, should I do this or will you?

[nickduncan7]

Feel free to take a crack at it if you'd like, I've already tried with the new URL API and it seems that the request doesn't get passed the full URL for some reason which makes the URL parsing fail in the new API. I'm still working on it, but it will probably be a few days until I can push anything into the repo.

[nahtnam]

Hey,

Sorry for the late reply... I did some research and stumbled upon nodejs/node#12682. Based on that, here are the possible solutions I found:

1. Relative URLs in WHATWG URL API nodejs/node#12682 (comment) (however the next comment says that that won't work. We could substitute something random like localhost in there
2. Use the url-parse library which has over 13 million downloads/week. Has two dependencies
3. Use the request-target library which has almost no downloads but is compliant with RFC 7230 and is supposedly more performant

I'd personally suggest method 2. It seems to be the most well-supported option.

[FranklinYu]

I think option 1 is fine. The comment below it says that you'll be in trouble when there is a backslash in path, but I think this case is rare. Are they even legal in HTTP request?

Since the “micro” framework is dealing with HTTP anyway, we can have dummy prefix like http://example.com/.

[nickduncan7]

Yes, that seems fine. I've implemented that solution and it seems to be working fine. @nahtnam I'll push this change into a separate branch, would you mind pulling it and making sure that it works?

I'll be adding full support for query parameters by simply passing the new WHATWG URL .searchParams object right onto the request object as .searchParams. I've written a new test to ensure this functionality works, but I'd like a second pair of eyes if possible before pushing it into NPM 😉

[nahtnam]

@protocol114 Looks good to me! Few nitpicky things but would it be better if we use localhost instead of example.com? I know it makes no difference but it feels "safer" if you get what I mean. Also not sure what sytnax you prefer, object destructuring over doing req.searchParams and stuff seems to be better.

Thanks!

[nahtnam]

Did you get a chance to review the changes/publish?

[nahtnam]

Another comment is with this line of code: https://github.com/protocol114/micro-http-router/blob/queryParams/src/router.js#L160

Just FYI that is a https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams object so you cant just do req.searchParams.host. You'd have to do req.searchParams.get('host'). I'm not sure you want to serialize it to JSON or not (I do it here https://github.com/ludicrousxyz/light/blob/master/src/query.ts#L10-L14) but just a heads up :)

[FranklinYu]

@nahtnam That serialization will eat repeated keys like ?sort[]=name+&sort[]=id-. I would suggest not serialize because JavaScript user is supposed to be familiar with URLSearchParams.

[nahtnam]

@FranklinYu Good catch. For some reason I thought URLSearchParams would return an array in situations like those. I have fixed the issue. If the user wants the URLSearchParams, it is available in req.searchParams as @protocol114 put in the branch. The point of my function is to convert it to a JSON object as that is what express does. Both options are available to the user as soon as that branch gets merged and published.

[nahtnam]

Sorry to keep bugging you guys, but any update? This is blocking some of my other work. I tried to publish my initial PR under @ludicrousxyz/micro-http-router but RunKit.com isn't detecting that package but it is picking this one up, so it would be nice if this package could just publish the changes.

Thanks!

[nickduncan7]

Sorry friend, been super busy with life things. I've got a quick change to make to my branch and I'll push it up and publish to npm sometime today.

[nickduncan7]

OK, sorry again. Just pushed up a new update 1.4.0 into npm that has the query param changes and a new router.unroute(path, method) function as per your previous issue as well. How badly do you need a remove all method? I could probably add that as well.

Let me know if everything's working correctly.

[nahtnam]

No worries, thank you for all of the efforts you put.

I do need either a unrouteAll or a way to get the list of all current routes (which I can then unroute myself`.

I am working on a framework which lets you hotswap routes so you don't have to wait for the server to restart every time you make a change (which can take a couple of seconds for big apps). But my issue is if someone changes a route from /a to /b, the /a route kind of just lingers until the server is fully restarted. It would be nice if I could deregister them. I could theoretically solve this by creating my own copy of a list of routes but it seems more efficient to do that here since the router already has the list of routes.

[nickduncan7]

I had some free time and added both an unrouteAll() and a router.routes object that lists all the current routes and which methods have handlers configured. Hope it's useful, go ahead and pull v1.5.0 from NPM and let me know if you encounter any issues!

[nahtnam]

Works perfectly, thank you!