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

should ignoreRoutes match any part of the url, or just the prefix? #189

Open
rsslldnphy opened this issue Aug 15, 2019 · 6 comments
Open

Comments

@rsslldnphy
Copy link

Ran into a problem earlier today while using the next-i18next middleware, which in turn uses this library. The next-i18next middleware defaults ignoreRoutes to ['/_next', '/static'] (as these are NextJS prefixes for static files). However we were seeing issues with urls of the form my.blog.com/posts/static-typing-is-cool also being ignored because they contained /static, rather than starting with it.

Had a quick look at the code and it looks like the ignoreRoutes are checked using indexOf rather than something like startsWith:

if (req.path.indexOf(ignores[i]) > -1) return next();

This is definitely not the behaviour I would expect - I would have expected it to be a prefix match - but as far as I can see it's not documented either way, so it's possible this was intentional for reasons I'm unaware of?

If not I'm happy to submit a fix in a PR. There are two approaches I can think of, each with drawbacks:

  1. Replace indexOf with startsWith - this would make ignoreRoutes behaviour match my intuition for what it should do. However, it would break things for any users of the library who relied on ignoreRoutes matching anywhere in the url rather than just the prefix. So probably not a great thing to do.

  2. Allow regular expressions as values for ignoreRoutes in addition to strings. This would allow users to choose whether they wanted prefix matching or matching anywhere in the url - anything a regex can do they can do. It would also allow leaving the current indexOf matching in place for strings, so avoid breaking anything for users who rely on this behaviour. However this could also be a drawback - as users who think they're getting prefix matching will carry on thinking that until they start seeing weird hard to debug errors on certain pages in their app like I did this morning 😄. Adding regexes as a third potential value for ignoreRoutes (along with strings and a function) would also add some more complexity.

On balance I think 2 would be the better option, because it can satisfy people who want a prefix match AND people who want to match tokens anywhere in the url. It could also maybe be a good idea to log a warning for people who have strings for ignoreRoutes to let them know it's not a prefix match and they can change it to a regex if they want to.

Let me know what you think and I'll put something together 🙂.

@jamuhl
Copy link
Member

jamuhl commented Aug 16, 2019

shouldn't i18next/next-i18next@a13e6f4 do the trick already...?

If you like go for 2) handle strings as handled right now..., if it's a Regex handle as regex

@rsslldnphy
Copy link
Author

Ah, should have perhaps made that clearer: the next-i18next issue is a related, but different issue. Adding a trailing slash to the config values fixes the worst cases, like my.blog.com/posts/static-typing-is-cool.

However even with the trailing slash, i18next-express-middleware will still ignore paths I would not expect it to, like my.blog.com/type-systems/static/haskell.

This is definitely less likely to happen than the previous examples, but it can still happen, and when it does, it's far from obvious why.

Happy to put a PR together implementing option 2. What do you think about logging a warning to let people know that their current config values are not prefix matches, and directing them to use the regex option if that is what they actually want?

@jamuhl
Copy link
Member

jamuhl commented Aug 16, 2019

I wouldn't warn...would be a little over the top for existing users which are happy with the current settings.

@rsslldnphy
Copy link
Author

yeah - i think the point of the warning is more that i would expect existing users are probably unaware of what their current setting is doing. going by the principle of least surprise. but at least now there will be this github issue to come up in google results.

@popod
Copy link

popod commented Jul 11, 2021

Same error for me..

For example: trying to only ignore route sitemap.xml but sitemap.xml.gz is ignored too !

How to manage that ? I think that @rsslldnphy solutions are good and should be implemented.

@adrai
Copy link
Member

adrai commented Jul 11, 2021

i18next-express-middleware is deprecated, use i18next-http-middleware instead, there the ignoreRoutes option can also be a function

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

No branches or pull requests

4 participants