feat(i18n): convert to opaque keys - #1848
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
📦 Bundle Size Report🎨 @videojs/html
Presets (7)
Media (10)
Players (5)
Skins (30)
UI Components (39)
Sizes are marginal over the root entry point. ⚛️ @videojs/react
Presets (7)
Media (9)
Skins (27)
UI Components (33)
Sizes are marginal over the root entry point. 🧩 @videojs/core
Entries (84)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (12)
📦 @videojs/spf — no changesEntries (4)
ℹ️ How to interpretJS sizes are initial static graph totals (minified + brotli). Lazy dynamic chunks are shown separately when present.
Run |
mihar-22
reviewed
Jul 16, 2026
Member
There was a problem hiding this comment.
this will drift, shouldn't this just import and use the values defined in i18n/text?
Collaborator
Author
There was a problem hiding this comment.
The text modules are generated from this file.
Member
There was a problem hiding this comment.
I see, I missed the comment at the top. Sounds good. Maybe we can make that shout more that the file is auto-generated?
Collaborator
Author
There was a problem hiding this comment.
Sure, do we have a standard header we can add perhaps? It's only a small line currently. I'll make it more obvious.
mihar-22
approved these changes
Jul 27, 2026
mihar-22
left a comment
mihar-22
left a comment
Member
There was a problem hiding this comment.
Thank you for resolving all of my comments, this looks great!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eb5024e. Configure here.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
As discussed in our meeting earlier, this moves over from using English as the key to opaque keys for the i18n locale files and usages. If we're happy with the DX of this change and the direction, then we can merge. If not, we can keep the existing English as a key. My preference would be to move to this but I realize there are aspects that weren't fully discussed earlier:
1. Drift
We now have the default text inline and the English locale catalogue and there's potential for them to drift - i.e.
<media-text token="buttons.play">Pause</media-text>ort('buttons.play', { default: 'Pause' })which could cause SSR issues along with confusion.We can solve this by adding a build-time consistency check that validates known keys against
en.ts- soPause!==Playwould throw. This can come in a later release if we decide to take this route.2. State-derived text
At the end of the meeting we discussed relying on CSS to hide/show multiple
<media-text>elements based on state like we do for icons, e.g.:becomes:
Notice we'd likely need to handle it in
<media-tooltip-label>as well since that currently callsgetResolvedLabel()on the trigger. If we're moving all of the state-derived text to the UI layer, we'd have to handle that too.Size changes
The increase is primarily from the locale migration rather than runtime logic:
The runtime additions are relatively small; most of the size comes from locale data, generated files, and the associated test coverage.
Note
Medium Risk
Wide i18n API and key-shape change across core and all locales; wrong or missing keys would break UI strings, though build-time locale completeness validation reduces that risk.
Overview
Replaces English-as-key locale strings with nested opaque keys (e.g.
buttons.play,errors.network) across all shipped locale packs and moves typing fromtypestoparamswith generatedTranslationParamsfor placeholder contracts.Build pipeline:
generate-i18n-localesnow flattens packs forloadLocale, validates every locale against English keys, and emits tree-shakeablei18n/text/*descriptors; a newgenerate-i18n-typesstep runs on prebuild. Package exports add./i18n/text/*.Runtime: Registry and browser translation work on
FlatTranslations; newTextdescriptors plusresolveText/translateTextsupport typed copy. Sandbox CDN checks use['buttons.play']instead of.Play.Reviewed by Cursor Bugbot for commit 7826717. Bugbot is set up for automated code reviews on this repo. Configure here.