-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove curry option and migrate to prototypes #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy new year! This is great! Left some minor comments that will probably allow us to make the bundle slightly smaller. Other than that this is 👌
index.js
Outdated
} | ||
} | ||
} | ||
function assertRoutename (routename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this function, and write these statements inline. This has two benefits:
- writing duplicate code compresses better. So while having more code in source, it'll probably end up being less code sent over the wire.
- this no longer works with static transforms such as unassertify and tinyify.
Hope this makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely. I didn't think about lz/gzip compression. 👍
I'll just use assert
then; that's basically already in use every setup and works better for the reason you mentioned.
index.js
Outdated
this.router = wayfarer(opts.default || '/404') | ||
} | ||
|
||
Nanorouter.prototype.on = function onRoute (routename, listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onRoute
variable name doesn't do anything in this code. Could we perhaps remove the variable names from all methods, and only keep the method names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added it to have a named function.
But usually nobody does this for prototype methods, nor do I usually 😁 .
ES6 improved anonymous functions a lot by defining the name automatically for the new syntaxes and let
and const
. But as we're still using prototypes, I'm good with removing it.
This module is quite small anyways and the function name of the constructor might be good enough.
@@ -0,0 +1,104 @@ | |||
var tape = require('tape') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really digging the tests! 🎉
New api: ``` const router = require('nanorouter')() router.on('/path', noop) // registers route router.emit('/path') // executes handler router.match('/path') // returns match {route: 'path', params: {}, cb: noop} ```
bae5d3e
to
4d9cbff
Compare
@yoshuawuyts Happy new year and thanks for the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! - sorry for the delay in reviewing, this is amazing :D
|
No problem. Thanks for your work. 🎉 |
The changes in here remove the curry option and migrate to prototypes as discussed in #12 and #6.
This pull request currently also includes a change done in #9. So we could do two releases (minor & major) or just a major one.
Api changes
curry
option got removed as mentioned inrouter.emit(path)
insteadMigration from the old
curry
optionThe
curry
option was confusing to use and read. Therefore we removed that completely.Previously it was possible to use the
curry
option like that:Now that this option doesn't exist anymore, we advise you to migrate to the
.match()
method as it's much more flexible in case you'll need more advanced parameters.if you won't have time to migrate your code and want to keep the old behavior, just wrap everything in a self-executing function.