Conversation
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 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 (69)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils
Entries (10)
📦 @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 |
sampotts
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3d1a240. Configure here.
| "sideEffects": false, | ||
| "sideEffects": [ | ||
| "**/i18n/registry.js" | ||
| ], |
There was a problem hiding this comment.
sideEffects omits bundled i18n entry
Medium Severity
sideEffects only lists **/i18n/registry.js, but the published @videojs/core/i18n entry is a single i18n bundle that inlines registry.ts (including pre-registering en). Bundlers that honor sideEffects may tree-shake away registry initialization when consumers import named exports from @videojs/core/i18n, leaving translations without the English base layer.
Reviewed by Cursor Bugbot for commit 3d1a240. Configure here.
| if (!expectedPlatform.has(tag)) { | ||
| warnings.push(`Unexpected locale re-export packages/${pkg}/src/i18n/locales/${file}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Workspace check crashes missing locale dirs
Low Severity
Check 7 calls readdirSync on packages/html/src/i18n/locales and packages/react/src/i18n/locales without verifying those directories exist. If locale stubs were not generated yet, pnpm check:workspace throws instead of reporting a clear warning like the per-file existsSync checks above.
Reviewed by Cursor Bugbot for commit 3d1a240. Configure here.
mihar-22
left a comment
mihar-22
left a comment
There was a problem hiding this comment.
Great work and thanks for doing this 😎
Overall this is feeling pretty good, but the thing giving me the biggest pause is how much i18n seems to be leaking down into component props, keys, and types. I’m also feeling some inconsistency in how the translation keys are set/formatted/named.
We’ve been talking a bit in the background about having a better text primitive, which feels related to one part of this: how do we simplify where text lives?
The other part is that I would have ideally liked to see something more like an optional translation function passed down through context. When available, components could use it. When not available, they could just fall back to the provided word or phrase. That feels like a cleaner separation of concerns to me, and it would also help with bundle size since loading the English locale by default feels a bit unfortunate. I think keys are creating some of that complexity right now.
I’m not opposed to exploring backing out of translation keys. The naming of them also feels a little unclear to me right now. It’s a mix of UI-specific keys like indicator*, menu*, errorDialog*, and then more generic words like play or mute. My gut says I’d prefer mostly plain words/phrases, unless there’s a strong reason the UI location matters for translation context.
Whatever direction we go with, I think the main things I’d like us to preserve are:
- it should be easy to understand where words live
- translating them should be straightforward when needed
- translation should stay completely optional (i should still be able to easily override any text on a component)
- i18n shouldn’t leak into component APIs/types
- we should be mindful of bundle size - ideally no need for a english translation map
I'm approving to unblock but it'd be nice if we can have some follow-up discussions/work on this front. Love to hear what you think about it all more too.
Maybe one follow-up for next time is whether we can split/chunk the CDN piece a bit more intentionally and add clear notes in PR description on how it would be consumed. I know it's somewhere in the design doc but that's harder to surface or have a clear/scoped conversation around.
| * | ||
| * @example `es-419-u-nu-latn` → `['es-419', 'es', 'en']` | ||
| */ | ||
| export function localeLookupChain(locale: string): string[] { |
There was a problem hiding this comment.
(suggestion): maybe we should move browser translation to /dom? we can then use lib types if they exist or extend globals.d.ts to clean up globalThis stuff here
| } | ||
|
|
||
| /** Registry map key: normalized tag with unicode extensions removed (same base as {@link localeLookupChain}). */ | ||
| export function canonicalLocaleRegistryKey(locale: string): string { |
There was a problem hiding this comment.
(suggestion): getCanonicalLocaleKey
| return out; | ||
| } | ||
|
|
||
| function mergeLookupChain(chain: string[]): Translations { |
There was a problem hiding this comment.
(suggestion): mergeI18nTranslations
| @@ -0,0 +1,132 @@ | |||
| import en from './locales/en'; | |||
| import type { Translations } from './types'; | |||
There was a problem hiding this comment.
Great if we can import and use TranslationKey type for this file.
| @@ -0,0 +1,29 @@ | |||
| function readLang(node: Element): string | undefined { | |||
| } | ||
|
|
||
| /** First non-empty `lang` on `start` or an ancestor (HTML language inheritance). */ | ||
| export function nearestLang(start: Element | null): string | undefined { |
There was a problem hiding this comment.
We could have a generic walk up utility that allows for a callback which this builds on.
| export function formatVolumePercent(fraction: number, locale?: string | string[]): string { | ||
| const value = !isNumber(fraction) || !Number.isFinite(fraction) ? 0 : Math.min(1, Math.max(0, fraction)); | ||
|
|
||
| try { | ||
| const formatter = getPercentFormatter(locale) ?? getPercentFormatter(undefined); | ||
| if (formatter) { | ||
| return formatter.format(value); | ||
| } | ||
| } catch { | ||
| // fall through to simple percent string | ||
| } | ||
|
|
||
| return formatVolumePercentFallback(value); | ||
| } |
There was a problem hiding this comment.
as per earlier comment, this doesn't belong in time/format
There was a problem hiding this comment.
maybe this function can be generalised, doesn't feel like a general/reusable utility at the moment
There was a problem hiding this comment.
i kind of get why this exists but it might be too advanced for our needs? love to see real-world examples
There was a problem hiding this comment.
This is to subscribe to the document lang - e.g. for sites with language selectors, this would ensure the player updates to match.
3 participants
Refs #1589
Refs #1590
Summary
Restores the combined i18n foundation and locale-pack work from the two accidentally merged PRs after #1707 reverted them out of main.
feat/i18nwill be the feature branch the other stacked PRs merge into and this PR will be used to merge the whole feature intomain:main← #1708
feat/i18n← #1591
feat/i18n-html← #1595
feat/i18n-react← #1593
feat/i18n-sandbox← #1600
docs/i18nNote
Medium Risk
Large, user-facing change to labels and accessibility across controls and many locales; risk is mitigated by tests, codegen checks, and restoring previously reviewed design, but regressions in string keys or locale loading would affect all skins.
Overview
Restores the Video.js 10 i18n stack that was reverted from main: a new
@videojs/core/i18nentry with a global registry, typedcreateTranslator,loadLocalelazy imports, 50+ shipped locale packs (v8 parity), and optional Chrome Translator API fallback (getBrowserTranslations/shouldAttemptBrowserTranslation).Codegen & CI:
generate-i18n-locales.tsruns on@videojs/coreprebuild, regeneratinglocales/all.ts,load-locale.ts, and html/react locale re-export stubs;check-workspacegains an i18n locales consistency check.package.jsonadds./i18nand./i18n/locales/*exports and marks the registry as a side effect.Player copy: English defaults live in
locales/en.tswith an expanded key set (live/cast, sliders, indicators, error dialog,timeRemainingPhrase, etc.). UI cores (e.g. captions/cast and others in the diff) now expose opaque label keys viaresolveOptionalControlLabelinstead of hardcoded English; related tests andinternal/design/i18n.mdare updated to implemented status and document provider merge order, tooltips, andloadLocale.Reviewed by Cursor Bugbot for commit 3d1a240. Bugbot is set up for automated code reviews on this repo. Configure here.