-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(i18n): load language pack asynchronously #34119
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
Conversation
Since forever we've been doing this dubious thing of passing the language pack through the dom. The server fetches the json file, and serializes it into a dom node as part of the "bootstrap data". Frontend picks it up deserializes it and hooks things up. In this PR: - creating a super simple flask endpoint to serve the json files - changing the "preamble" to run an async call to fetch the language pack before doing anything
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Redundant Initial Configuration ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/preamble.ts | ✅ |
| superset/views/base.py | ✅ |
| superset/views/core.py | ✅ |
| superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| import { User } from './types/bootstrapTypes'; | ||
| import getBootstrapData, { applicationRoot } from './utils/getBootstrapData'; | ||
|
|
||
| configure(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
msyavuz
left a comment
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.
LGTM!
| import { User } from './types/bootstrapTypes'; | ||
| import getBootstrapData, { applicationRoot } from './utils/getBootstrapData'; | ||
|
|
||
| configure(); |
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.
What is this for? Is there a loading happening between this and the language pack loading?
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'm unclear what exactly it does, but it prepares a bunch of things around the various "registries" and such. It logs a bunch of things to the console if called too late. Might be worth digging a bit deeper. I'll double check there's no important perf costs in calling it twice.
|
|
||
| @event_logger.log_this | ||
| @has_access | ||
| @expose("/language_pack/<lang>/") |
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.
@mistercrunch It would be nice to put this endpoint under /api/v1 to be an official endpoint.
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.
yeah I thought through this wondering if this is a private or public endpoint, and thought ideally should be public, but given the partitioning of libraries in the frontend, and the fact that our i18n framework is monolithic currently (no segmentation or context based on the provenance of the strings), it felt like it would require significantly more work to do this properly.
Not sure whether the extension framework SIP mentions i18n, but it may be a bit of a blindspot.
Without going to deep into it, it seems that if we want for extensions to be localized/localizable, we'd need some way to segment the language pack based on package provenance (backend/frontend, and break down per library on the frontend side). The whole i18n stack might need to offer different ways to fetch and apply translations.
Happy to help think through this, but it's a fairly significant amount of work.
Note that one minor and valuable thing I considered while doing the work here was to find a way to add metadata to translation to identify what is frontend/backend, and make sure the endpoint only returns the strings intended for the frontend ... The gettext-based/flask-babel framework we use does offer ways to do such things, but I encountered some limitations, and decided to punt on these.
This PR here addresses something pretty important - removing the language pack from bootstrap data - but there's much more to do in this area.
Since forever we've been doing this dubious thing of passing the language pack through the dom. The server fetches the json file, and serializes it into a dom node as part of the "bootstrap data". Frontend picks it up deserializes it and hooks things up.
In this PR: