Skip to content
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

Match start with path (*) #6

Merged
merged 5 commits into from
May 17, 2017
Merged

Conversation

billouboq
Copy link
Contributor

Help using fastify middleware when you want to match multiple routes with same starting url.

Needed it to switch from express :

router.all('/api/*', yourMiddleware);

to

fastify.use('/api/*', yourMiddleware);

middie.js Outdated
functions[i](req, res, that.done)
} else if (typeof urls[i] !== 'string' && urls[i].indexOf(url) > -1) {
functions[i](req, res, that.done)
} else if (endOfPathAsterixRegex.test(urls[i]) && url.indexOf(urls[i].slice(0, -1)) > -1) {
Copy link
Member

@mcollina mcollina May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regexps are fairly expensive, I think we should avoid this regexp check for every request, but rather do it once. Maybe we need to change how we store functions and urls.

Regarding the indexOf and slice, we might have to write our own little functions that does a charcode-by-charcode comparison, as the slice call might be too expensive?

Copy link
Contributor Author

@billouboq billouboq May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a fairly expensive call, I think we should avoid this regexp check for every request, but rather do it once.

Don't know how you want to implement that, do you want to store it on instance with something that say "this middleware use * in route" ?

Regarding the indexOf and slice, we might have to write our own little functions that does a charcode-by-charcode comparison, as the slice call might be too expensive?

will dig into this later then, thanks for the review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can store both urls and functions in a literal, with a property that tells if that has a wildcard or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree with @mcollina, regex are too expensive. You can check how we are handling wildcards in our router, but this case is a bit different.

@billouboq
Copy link
Contributor Author

@mcollina @delvedor changed implementation what's your thoughts?

middie.js Outdated
} else if (urls[i].wildcard && pathMatchWildcard(url, urls[i].path)) {
urls[i].fn(req, res, that.done)
} else if (urls[i].path === url || (typeof urls[i].path !== 'string' && urls[i].path.indexOf(url) > -1)) {
urls[i].fn(req, res, that.done)
Copy link
Member

@mcollina mcollina May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can assign urls[i] to a variable, as we are accessing it everywhere.

urls[i].fn() will set this to urls[i], maybe this was happening before as well, but I think we should change it.

Copy link
Contributor Author

@billouboq billouboq May 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About naming, feel to change whatever seems unclear to you. I always have hard times giving names.

urls[i].fn() will set this to urls[i], maybe this was happening before as well, but I think we should change it.

Not sure which context we want to assign to that function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@billouboq just fn = urls[i].fn, fn()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should store urls[i] in a variable and not only fn, since fn is called just one time, while urls[i] is called several times :)

@mcollina
Copy link
Member

Good work! We are almost there.

@billouboq
Copy link
Contributor Author

add variables and changes names to be more understandable.

@mcollina
Copy link
Member

This is perfect, thanks!

@mcollina
Copy link
Member

I've checked the benchamarks of fastify as well, and this does not introduce any overhead in the base cases.

@mcollina mcollina merged commit e9e57e0 into fastify:master May 17, 2017
@mcollina
Copy link
Member

can you please send us a PR to the README as well? I think we should cover this there too.

@billouboq billouboq deleted the match-start-with branch May 17, 2017 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants