-
Notifications
You must be signed in to change notification settings - Fork 901
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
feat(app): Use i18next-resources-to-backend instead of i18next-http-backend #6717
Conversation
Why is the python stuff using the app files... 👀 |
A lot of internal tools are using the country names from en.json. I am wondering if we should first try to separate that out, so it's available in the config somewhere instead? Right now, any change to |
Just the zone names? I had planned on doing that as a part of a later change which would use namespaces (that means we can separate out the core translations, zone names and say the FAQ into their own files. But would it be enough to just leave a copy of the en.json file in the old location for now? |
…ources-to-backend
When we change a UI translation in As first I thought about having them in the Python configuration files instead, but then we would have to duplicate it for the app to use. But maybe we can come up with another solution? I honestly think we could even consider having the English name for each zone directly in the zone YAML files :) |
I think the namespace solution would be the best solution without duplicating anything and without breaking any translations. Then the zones names would be in their own file which would basically never change. But I think that should be a follow up change to this PR to try and stick to the idea of perfect PRs (only one change per PR). But I'm also not totally against the idea of doing it here. |
I agree it should be a separate PR :) |
Earthfile
Outdated
COPY web/src/locales/en.json ./web/public/locales/en.json | ||
COPY __init__.py ./__init__.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to update the dest
path as well, and update it everywhere it's used in monorepo - otherwise there will be a discrepancy between running things on your local machine vs on CI :)
COPY web/src/locales/en.json ./web/public/locales/en.json | |
COPY __init__.py ./__init__.py | |
COPY web/src/locales/en.json ./web/src/locales/en.json | |
COPY __init__.py ./__init__.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do de even need to copy them then? Don't we copy the entire src folder?
My thinking here was that we could merge this without impacting other deployments and then point them to the new file location if needed. But as you say it wouldn't match local development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other project use this +src-files target, which includes translations but not the whole app src folder. Otherwise they would rebuild all the time 😅
used like this: COPY ../../contrib+src-files/* /contrib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh god 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is why I thought it might be better to first split it out, and then afterwards do this change 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but splitting it out would require other changes as well...
Could we just not leave a copy in the old location for now (maybe with just the zone names) and then migrate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could, then we should remove anything but zone names to avoid confusing contributors :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also make sure we update documentation in various places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the Wiki after this has been approved (but right before I merge it) so we don't cause a mismatch for any longer period of time.
Sentry Issue: APP-WEB-31W |
…ources-to-backend
Let's wait a bit with merging this so it gets it's own release in case something in the backend acts up :) |
Note to self: EDIT: |
Issue
The current NPM package we use for a i18next backend is designed for server side translations and are causing some issues with requests failing (and falling back to the index.html) when the language don't exactly match.
For example
en-US
would fail and then fall back toen
which would work.The current package,
i18next-http-backend
, also contains a dependency oncross-fetch
which are causing an additional 20kb of code to be included in our index bundle bloating the app.We also have a problem with the JSON files not being minimised due to being in the public folder.
Closes AVO-78
Description
This PR switches out the
i18next-http-backend
package fori18next-resources-to-backend
and moves the language files to thesrc
directory. This together will bundle the JSON files as JS files and generate dynamic imports for them instead. Since it is now passing though the vite optimization engine it ensures the files get minimised as well.Important
Since the api relies on the same translations as the app take some extra care to double check the modifications to the Earthfiles.
This change will also help facilitate the implementation of a service worker as it will have built in cache busting which removes the need to hash the files in the service worker instead.
Preview
On master:
On this branch:
Double check
pnpx prettier@2 --write .
andpoetry run format
in the top level directory to format my changes.