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

Default config ignores any pages that have a path token that starts with static #455

Closed
rsslldnphy opened this issue Aug 15, 2019 · 3 comments · Fixed by #457
Closed

Default config ignores any pages that have a path token that starts with static #455

rsslldnphy opened this issue Aug 15, 2019 · 3 comments · Fixed by #457

Comments

@rsslldnphy
Copy link
Contributor

rsslldnphy commented Aug 15, 2019

Describe the bug

Default values for the ignoreRoutes option include /static without a trailing slash: https://github.com/isaachinman/next-i18next/blob/09be96fad33d37f38997cf89103a4b7d690518a8/src/config/default-config.ts#L29

Presumably this is intended to prevent i18n from being loaded for static files - however as the code checks these values with startsWith:
https://github.com/isaachinman/next-i18next/blob/09be96fad33d37f38997cf89103a4b7d690518a8/src/middlewares/next-i18next-middleware.ts#L21
it also prevents loading of i18n for any routes that start with /static. Such as, for example: my.blog.com/static-typing-is-cool.

For us it also seems to happen when any path token at all starts with static: so my.blog.com/posts/static-typing-is-cool would also be ignored. Haven't gotten to the bottom of this.

Occurs in next-i18next version

We're using 0.44.0 but as you can see from the links above, the code that causes this is still in master.

Steps to reproduce

Using i18next without overriding the default ignoreRoutes option, and creating a page, wrapped in the i18n middleware, whose url starts with /static, will reproduce this bug.

Expected behaviour

Only paths that start with /static/ or /_next/ should be ignored by default. It may be that custom asset prefixes also need to be taken into account to make this work, too..

OS (please complete the following information)

N/A

Additional context

Happy to submit a PR that changes the default config values but don't know enough about the internals of this library to know if that's likely to have any unintended consequences.

@isaachinman
Copy link
Contributor

The most we could do here is add a trailing slash to these substrings, eg /static/. These subpaths are NextJs conventions, and we need to exclude this traffic from the middleware to prevent locale subpath redirection.

The reason ignoreRoutes is included in the config is to allow users to opt out of this. Not sure this is actually a "bug", but happy to hear clear suggestions about how we can improve.

@rsslldnphy
Copy link
Contributor Author

I don't mean to be rude, but this is fairly obviously a bug.

First, because what the code intends to do (ignore static files) and what it actually does (ignore static files AND all pages that have a path segment starting with static) are very different;

Secondly, because I spent an hour this morning "debugging" why our app was blowing up whenever someone wrote a post with a title that started with the word "static". It was only the intuition to search this codebase for use of the startsWith function that led me to the root cause.

If the contract of this library is "you can have i18n but you can no longer have any pages with path segments that start with the word static then a) that's a pretty strange contract and b) it should be made very clear in the readme that that's the case. It was certainly non-obvious to me why i18n was present on every page on our site except a select handful where it was, for no clear reason, undefined.

I think I've already expressed the issue pretty clearly, expressed the "expected behaviour" (i.e. how the library can improve) pretty clearly, pointed directly to the lines of code causing the issue, and offered to do the work myself to make a pull request to fix it. So I'm not sure how i could have been a better open-source citizen in this case, and I'm confused as to why you feel the need to tell me that the bug I'm reporting is not a "bug", and that you need some "clear suggestions" about how to improve other than the clear suggestions already in the bug report?

But if it helps:

  1. next-i18next should, by default, only ignore routes that start with either /static/ or /_next/
  2. routes that start with /static, such as /static-typing-is-cool should not be ignored
  3. routes that contain path segments that start with static, such as /blog/static-typing-is-cool should not be ignored

And to reiterate, I'm more than happy to put together a pull request that makes these fixes. I don't know the internals of this library, so I thought I would call your attention to the issue before attempting to make a fix in case there might be any unintended consequences I would not have foreseen.

@isaachinman
Copy link
Contributor

PRs welcome.

rsslldnphy added a commit to rsslldnphy/next-i18next that referenced this issue Aug 15, 2019
isaachinman pushed a commit that referenced this issue Aug 16, 2019
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 a pull request may close this issue.

2 participants