search

i18next / i18next-express-middleware

[deprecated] can be replaced with i18next-http-middleware
https://github.com/i18next/i18next-http-middleware
MIT License
206 stars 52 forks source link

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

Open rsslldnphy opened 5 years ago

rsslldnphy commented 5 years ago

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: https://github.com/i18next/i18next-express-middleware/blob/ea4989b183b31ad5bd58e7a6b71300f722f45cd5/src/index.js#L15

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 commented 5 years ago

shouldn't https://github.com/isaachinman/next-i18next/commit/a13e6f42e5dc32796980b3aeb343ea68200c3de4 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 commented 5 years ago

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 commented 5 years ago

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

rsslldnphy commented 5 years ago

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 commented 3 years ago

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 commented 3 years ago

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