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. |
📦 Bundle Size Report🎨 @videojs/html — no changesPresets (7)
Media (6)
Players (3)
Skins (16)
UI Components (21)
Sizes are marginal over the root entry point. ⚛️ @videojs/react — no changesPresets (7)
Media (5)
Skins (14)
UI Components (18)
Sizes are marginal over the root entry point. 🧩 @videojs/core — no changesEntries (7)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (10)
📦 @videojs/spf — no changesEntries (3)
ℹ️ How to interpretAll sizes are standalone totals (minified + brotli).
Run |
| ## Consumer Usage | ||
|
|
||
| ### Static translations | ||
|
|
||
| ```tsx | ||
| import { VideoSkin } from '@videojs/react/video'; | ||
| import { esTranslations } from './locales/es'; | ||
|
|
||
| <VideoSkin locale="es" translations={esTranslations} src={src} /> | ||
| ``` | ||
|
|
||
| ### Built-in translations | ||
|
|
||
| Video.js ships locale packs for common languages. Set `locale` and the matching pack loads automatically — no `translations` prop needed: | ||
|
|
||
| ```tsx | ||
| // Built-in Spanish translations load lazily on mount | ||
| <VideoSkin locale="es" src={src} /> | ||
| ``` | ||
|
|
||
| The pack is loaded via dynamic `import()` so it has zero impact on the default bundle. If no pack exists for the requested locale, the player falls back to English. | ||
|
|
||
| Consumer-provided `translations` always take priority over built-in packs: | ||
|
|
||
| ```tsx | ||
| // Built-in "es" pack loads, then "Play" is overridden by the consumer value | ||
| <VideoSkin locale="es" translations={{ Play: 'Reproducir' }} src={src} /> | ||
| ``` | ||
|
|
||
| For SSR or when you want zero flash-of-English on first render, pre-import the JSON and pass it directly: | ||
|
|
||
| ```tsx | ||
| import es from '@videojs/core/i18n/locales/es.json'; | ||
|
|
||
| <VideoSkin locale="es" translations={es} src={src} /> | ||
| ``` |
There was a problem hiding this comment.
There's a tiny bit of awkwardness, here, having two props that essentially describe the same thing. locale="es" translations={es}. I think I see how we got here. Like, we don't know if translations is a partial, so we need locale to fall back on. But. Just. Calling it out.
There was a problem hiding this comment.
They're for different uses but maybe the naming in the example is tripping you up.
locale(orlangas folks are pushing for) is used for Intl API calls to format numbers and duration, get plural rules etc.translationsis providing a dictionary of translation overrides.
Are you maybe suggesting we handle loading the JSON so they don't have to provide the translations prop here?
cjpillsbury
left a comment
cjpillsbury
left a comment
There was a problem hiding this comment.
Although I threw out a few "blocking" comments, I trust the rest of the team to address/discuss sufficiently and I will be out for a long weekend and don't want to slow forward progress, so setting this review as "Comment" only.
| ```tsx | ||
| import { VideoSkin } from '@videojs/react/video'; | ||
|
|
||
| <VideoSkin |
There was a problem hiding this comment.
question(blocking): Have there been any discussions on the tradeoffs of including the provider in the skin vs. elsewhere, e.g.:
- bundled with our MediaProvider, which can still be two distinct providers "under the hood"
- a standalone provider that folks can opt in to use as they see fit
- something else?
Concerned about this breaking our broad pattern of not adding too much "configuration" to a Skin. (BYO Skins, Skins without labels if they want, etc. etc.)
There was a problem hiding this comment.
This is another aspect I went back and forth on. Happy to discuss next week (I'll setup a call) but I figured the skins may own the translations it requires. That said given our eject stance, we could just have "core" translations that cover all of our skins requirements and ejected folks can opt to use our i18n or (more likely) their own implementation.
There was a problem hiding this comment.
What I'm not keen on is:
<AProvider>
<BProvider>
<CProvider>
<DProvider>
{children}
</DProvider>
</CProvider>
</BProvider>
</AProvider>😆
There was a problem hiding this comment.
Yeah, per usual, I'm more concerned about "fundamentals" and architectural assumptions. I get not wanting ☝️ (although that has become the "accepted" norm in most of React land, which is what I'm assuming is top of mind with your example). I do think we want to keep i18n (and likely other categories of state) as separate providers from the core media state, it's easy enough to define a "Provider" as a thin wrapper around multiple Providers if that's the preferred balance between devex vs. composition
internal/design/i18n/decisions.md
Outdated
|
|
||
| ## One `Translations` type, not two | ||
|
|
||
| **Decision.** There is a single `Translations` interface that carries both the index signature (`[key: string]: string | undefined`) and all the named player keys. Skins extend it directly with their own keys. There is no separate structural base type. |
There was a problem hiding this comment.
question(non-blocking): I'm wondering if we should make this (in the TypeScript sense), an extendable set of well-defined string literals instead of simply string? I suspect @mihar-22 could help you with that.
There was a problem hiding this comment.
If we're not going to both with skin-specific translations, this would be miles easier for sure. I'm all for strongly typed where possible.
internal/design/i18n/decisions.md
Outdated
|
|
||
| - **Separate base interface** (e.g. `BaseTranslations` with only the index signature, `Translations extends BaseTranslations` with the named keys). An extra type and export whose only job is to donate an index signature. | ||
|
|
||
| **Rationale.** A base interface that exists solely to donate an index signature is needless indirection — the index signature belongs on the type that consumers actually work with. The HTML `i18nContext` previously typed to a structural base (`Translator<BaseTranslations>`) now uses `Translator<Translations>` directly, which is equally assignable from any `Translator<SkinTranslations>` since `SkinTranslations extends Translations`. |
There was a problem hiding this comment.
question(non-blocking): My concern here is that there will be no "hints" for folks on whether a particular "key" is valid. This can easily result in dev toil/maintenance difficulties in both using the API and defining a JSON object for a new lang.
There was a problem hiding this comment.
The component builders should still get those hints right. A base would only be handy for folks creating a new language dict but you can just copy the english dict for example and start changing values.
|
|
||
| ## Native `Intl` APIs for locale-aware formatting | ||
|
|
||
| **Decision.** Use `Intl.DurationFormat` for time phrases, `Intl.NumberFormat` with `style: 'percent'` for volume values, and `Intl.PluralRules` (via `pluralize`) for plural selection — with translation-key fallbacks for environments that lack `Intl.DurationFormat`. |
| - **Pass `Translator` into each Core class** — via constructor or `setProps({ t })`. Core is the single source of truth for all string production. | ||
| - **Add `translate?` param to `getLabel` and `getAttrs`** — Core calls `translate?.('Play') ?? 'Play'` internally. | ||
|
|
||
| **Rationale.** Core is runtime-agnostic and framework-independent. Threading a context-derived translator into Core would create implicit coupling to whichever context system the caller uses (React context, DOM context, or a test double). The UI layer already holds the translator and is the natural place to apply it — it is the same layer that decides which attributes to set on the element. Core producing the key and the UI layer calling `t(key)` keeps the layers clean. |
There was a problem hiding this comment.
question(non-blocking): I know we've gone down the path of expecting folks to pass in all labels etc, but I think this will create a lot of noise/burden on the user for the "standard use case". I still think these should be treated more like our built in state management, and they can be directly related to identical logic as state changes (e.g. showing both "Play" and the play icon in paused state; showing "Pause" and the pause icon in paused state). We weave that context through core already and have already solved those hard problems. That said, I believe this can be a future effort and there are no inherent "left turns instead of rights", so I don't think this should be a blocker.
|
|
||
| ## Separate `i18nContext` for HTML, not extending `playerContext` | ||
|
|
||
| **Decision.** A new `i18nContext` is created alongside `playerContext`, `mediaContext`, and `containerContext`. |
There was a problem hiding this comment.
Agree. This is a different context with different responsibilities. However (non-blocking), we may want well-defined hooks that "know about" both contexts. This relates to my earlier comment about state, above, and was related to our earlier conversations on props hooks, react-aria/adobe spectrum points of inspiration, and the like. e.g. something like usePlayButtonProps(state, props) having reference to the I18N context. Like above, this is non-blocking, since nothing you're proposing precludes or "code away from" this in a future effort.
|
|
||
| ## `I18nMixin` on the skin element, not a separate provider element | ||
|
|
||
| **Decision.** `VideoSkinElement.translations` triggers re-provision to all descendants via `I18nMixin`. |
There was a problem hiding this comment.
issue(blocking): This I think pretty strongly is a wrong turn, for a few reasons.
- It creates a hard coupling of skin to i18n, meaning you need to use a skin to use i18n
- It creates a hard coupling of i18n to a UI component, meaning you need to use a particular UI component to use (data/state) i18n
- It's a break in our general "compositional" ethos
- It creates an unnecessary disparity between React vs. HTML "for the wrong reasons". While we're okay with divergences, those should be primarily motivated by the idioms of the platform/framework/etc.
- It creates an unnecessary disparity between React vs. HTML that will complicate any future possible compiler work. While this shouldn't be a primary motivator for decisions (i.e. our general stance has been that we should make the "right" decisions and deal with any compiler consequences as needed), it's worth surfacing if there are already other concerns on the decision.
There was a problem hiding this comment.
Sure, can change this. As above, perhaps we don't need to allow our own skins to have their own translations anyway as it just complicates things needlessly. Something to also discuss.
|
|
||
| ## Built-in locale packs, lazy-loaded | ||
|
|
||
| **Decision.** Ship JSON translation files for common languages in `@videojs/core/i18n/locales/`. Load them dynamically when `locale` changes; merge with consumer `translations` (consumer always wins). |
There was a problem hiding this comment.
question(non-blocking): Deeply assuming this and only this does have ripple effects that I think are worth talking through, like how locales are written, stored, made available, build tools, https://caniuse.com/mdn-javascript_statements_import_import_attributes_type_json support, etc. etc. I'm interested in others' thoughts here. This also abuts my (out of band) question re: async translate() and optional fallbacks on e.g. https://developer.mozilla.org/en-US/docs/Web/API/Translator if/when available in the future.
|
|
||
| ### How it works | ||
|
|
||
| The store's `textTrackList` already captures `language` (mapped from the DOM `TextTrack.language` / `srclang` attribute) for every track. When `locale` is set, a new store feature compares it against the track list and activates the best match. |
There was a problem hiding this comment.
issue(blocking): I think any textTrackList should be out of scope for this effort. @heff + @luwes (and also @gkatsev) and I spent a lot of time talking through the difference between (1) "Application" language settings and preferences versus (2) "user" language settings and preferences versus (3) "default stream" language details. What you're working on primarily is (1) and is typically defined by app designers/developers (even if they expose a UI for "locale selection". (2) is generally deferred to things like navigation.language and the like, with the possibility of localStorage or similar for a dynamic "user preferences" model. (3) is typically defined in a way (per spec) that provides a "default" (e.g. subtitle, audio, etc.) language for a src, but also "assuming no detectable user preferences". E.g. from the HLS/Pantos/RFC8216 spec:
DEFAULT The value is an enumerated-string; valid strings are YES and NO. If the value is YES, then the client SHOULD play this Rendition of the content in the absence of information from the user indicating a different choice. This attribute is OPTIONAL. Its absence indicates an implicit value of NO.
I'd just omit this section/effort altogether for now.
There was a problem hiding this comment.
Also thanks @mister-ben for calling this out on Discord! 👯
There was a problem hiding this comment.
Good call. This can come later.
|
|
||
| ## English strings as translation keys | ||
|
|
||
| **Decision.** Translation keys are the English strings themselves. `t('Play')` not `t(TranslationKey.PLAY)`. |
There was a problem hiding this comment.
We came to the same conclusion in media-chrome. Makes a lot of sense and should also help gzip.
muxinc/media-chrome#1071 (comment)
There was a problem hiding this comment.
thought(non-blocking): Yeah this is pretty different from how it typically works in other (non React-y) envs and I expressed similar sentiments when this has come up in the past, but not a hill I need to die on 😅.
Examples where these are well-defined keys:
- ICU CLDR (ignore the deep semantic structure, as that is irrelevant to the core callout) - https://github.com/unicode-org/cldr-json/blob/main/cldr-json/cldr-person-names-full/main/aa-DJ/personNames.json#L56-L59
- Android Applications - https://developer.android.com/guide/topics/resources/localization#managing-strings
- iOS Applications - https://developer.apple.com/documentation/xcode/localizing-and-varying-text-with-a-string-catalog
(there are more, and there are reasons to not conflate presentation with data, esp if we don't want to assume the english phrasing will be "set in stone", as that has p big ripple effects, esp on 3rd party BYO language keys, but I'm happy to let this be y'allz decision in the end)
internal/design/i18n/architecture.md
Outdated
|
|
||
| ```tsx | ||
| // Consumer API — just props on the skin | ||
| <VideoSkin locale="es" translations={{ Play: 'Reproducir', 'Skip Intro': 'Saltar Intro' }} /> |
There was a problem hiding this comment.
Could we consider the lang prop / attr here?
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/lang
We used this in Media Chrome, it can go on every html tag and gives the browsers extra context.
Or is there a reason not to meddle these two?
| * Any BCP 47 locale tag. Known built-in locales autocomplete in editors; | ||
| * any other tag (e.g. 'pt-BR', 'zh-TW', 'en-AU') is accepted without a cast. |
There was a problem hiding this comment.
BCP 47 tags don't necessarily match the pattern of the examples given. zh-Hant-TW, es-419, yor, sr-Latn, en-GB-scotland. Will these be accepted too?
There was a problem hiding this comment.
Yep, that's the plan. I can add some more examples in here.
| --- | ||
|
|
||
| ## Parametric cores return the template key, not the resolved string | ||
|
|
There was a problem hiding this comment.
question(non-blocking): This feels counter-intuitive and makes a hard requirement for those not using our i18n infrastructure. In other words, we've just lost the value add for the "simple case" and replaced it with a dependency on the "more advanced case". I won't dig my heels in on this, but it feels like some of the complications here are because we're deciding to use the presentation template string as both "key" and (unresolved) value, some of the complications here are because we've moved away from having i18n considerations more deeply integrated into the components (a la react-spectrum inspiration crud we were exploring/aiming toward last year). This also creates a steeper curve as folks start trying to use VJS "more closely to the metal" (which I think has been a theme of some of our decisions in 2026). Food for thought.
5 participants
Summary
Add a design document for the i18n (internationalization) system, capturing the architecture decisions and rationale for how Video.js 10 will handle localization across its monorepo packages.
Changes
Testing
Documentation only — no code changes.
Note
Low Risk
Low risk: documentation-only additions with no runtime or build output changes.
Overview
Adds a draft internal i18n design doc set (
internal/design/i18n/) covering the proposed translation architecture (corecreateTranslator+ locale typing, React hook factory/provider pattern, HTML context/mixin/controller approach, and utils helpers likepluralize/formatDuration).Includes a decision log capturing key tradeoffs (English-as-key, per-skin typed keys, lazy-loaded built-in packs, optional browser Translator API fallback, SSR locale/hydration guidance) and an index/quick-start reference tying the docs together.
Written by Cursor Bugbot for commit 669c88c. This will update automatically on new commits. Configure here.