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

Only overwrite ns config if it provided #2270

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Conversation

NamPNQ
Copy link
Contributor

@NamPNQ NamPNQ commented Apr 10, 2024

Now we require including serverSideTranslation to every page that needed to translate

With this change, we can ignore that behavior by using all namespace available

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

@adrai
Copy link
Member

adrai commented Apr 10, 2024

I don't really understand what this should exactly change...
Maybe create a reproducible example?
Can you please provide a new test case that fails without that change and succeeds with this change?

@NamPNQ NamPNQ changed the title only overwrite ns when createClient if it present, else we will default ns from createConfig [wip]only overwrite ns when createClient if it present, else we will default ns from createConfig Apr 10, 2024
@NamPNQ NamPNQ changed the title [wip]only overwrite ns when createClient if it present, else we will default ns from createConfig Only overwrite ns config if it provided Apr 11, 2024
@NamPNQ
Copy link
Contributor Author

NamPNQ commented Apr 11, 2024

@adrai I just update my PR, can you take a look, I split this to every step for each commit, and add example
The example of auto-static-optimize is just copied from simple, modify config in _app, next-i18next.config and static page without getStaticProps

@adrai
Copy link
Member

adrai commented Apr 11, 2024

The tests are failing...
and I still don't get why ...(ns && { ns }) should change anything...
btw: I get this error with your example:
image

Copy link

socket-security bot commented Apr 12, 2024

@NamPNQ NamPNQ force-pushed the patch-2 branch 2 times, most recently from f40fc70 to 2f9a93d Compare April 13, 2024 08:48
@NamPNQ
Copy link
Contributor Author

NamPNQ commented Apr 13, 2024

@adrai
Replacing {ns} with {...(ns&&{ns}} was necessary due to server-side behavior, as evident in the code at https://github.com/i18next/next-i18next/blob/master/src/config/createConfig.ts#L254. When ns isn't provided, the server loads all namespaces. However, for auto static, ns isn't defined in the configuration, resulting in it being set to undefined. Consequently, no translations occur.

Currently, I'm not having any a solution for handling this mismatch. Since rendering occurs on the server side, while the client side lacks namespace information for preloading, translations won't occur, leading to a mismatch.

For test fail, because I'm missing add build:example for that

@adrai
Copy link
Member

adrai commented Apr 13, 2024

The mismatch is exactly because setversideTranslations is not called... so this will never work.

The mismatch needs to be resolved else I can't merge this PR.

@NamPNQ
Copy link
Contributor Author

NamPNQ commented Apr 15, 2024

I update to use config ns and initImmediate: false to fix mismatch, the mismatch only happens in cache first time (the translation not fetched) or the cache expired

@adrai adrai merged commit 0736495 into i18next:master Apr 15, 2024
5 checks passed
@adrai
Copy link
Member

adrai commented Apr 15, 2024

it's included in v15.3.0

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.

2 participants