fix(html): preserve side-effect imports and fix context upgrade order#691
Closed
fix(html): preserve side-effect imports and fix context upgrade order#691
Conversation
tsdown unbundle mode was stripping bare side-effect imports in composite define files, preventing child custom elements from being registered. Additionally, the import chain caused `<media-container>` (consumer) to upgrade before `<video-player>` (provider), so the @lit/context handshake failed silently — the store never attached and controls threw NO_TARGET. Fix tree-shaking by converting bare imports to re-exports. Fix upgrade order by extracting the container class to a separate module and registering it after the player in each player define file. Closes #688 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for vjs10-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📦 Bundle Size Report
Total: 53.75 kB · 0 B · 0% Entry BreakdownSubpath sizes are the additional bytes on top of the root entry point, measured by bundling root + subpath together and subtracting the root-only size.
|
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
. |
4.28 kB | 4.28 kB | 0 B | 0% | ✅ |
./dom |
6.01 kB | 6.01 kB | 0 B | 0% | ✅ |
| total | 10.28 kB | 10.28 kB | 0 B | 0% |
@videojs/element
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
. |
817 B | 817 B | 0 B | 0% | ✅ |
./context |
823 B | 823 B | 0 B | 0% | ✅ |
| total | 1.60 kB | 1.60 kB | 0 B | 0% |
@videojs/html
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
. |
15.20 kB | 15.20 kB | 0 B | 0% | ✅ |
./video |
1.06 kB | 1.06 kB | 0 B | 0% | ✅ |
./audio |
1.06 kB | 1.06 kB | 0 B | 0% | ✅ |
./background |
1.05 kB | 1.05 kB | 0 B | 0% | ✅ |
| total | 18.36 kB | 18.36 kB | 0 B | 0% |
@videojs/icons
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
./react |
2.27 kB | 2.27 kB | 0 B | 0% | ✅ |
./html |
1.52 kB | 1.52 kB | 0 B | 0% | ✅ |
| total | 3.79 kB | 3.79 kB | 0 B | 0% |
@videojs/store
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
. |
1.29 kB | 1.29 kB | 0 B | 0% | ✅ |
./html |
468 B | 468 B | 0 B | 0% | ✅ |
./react |
204 B | 204 B | 0 B | 0% | ✅ |
| total | 1.95 kB | 1.95 kB | 0 B | 0% |
@videojs/utils
| Entry | Base | PR | Diff | % | |
|---|---|---|---|---|---|
./array |
104 B | 104 B | 0 B | 0% | ✅ |
./dom |
928 B | 928 B | 0 B | 0% | ✅ |
./events |
227 B | 227 B | 0 B | 0% | ✅ |
./function |
261 B | 261 B | 0 B | 0% | ✅ |
./object |
119 B | 119 B | 0 B | 0% | ✅ |
./predicate |
265 B | 265 B | 0 B | 0% | ✅ |
./string |
148 B | 148 B | 0 B | 0% | ✅ |
./style |
185 B | 185 B | 0 B | 0% | ✅ |
./time |
478 B | 478 B | 0 B | 0% | ✅ |
./number |
158 B | 158 B | 0 B | 0% | ✅ |
| total | 2.81 kB | 2.81 kB | 0 B | 0% |
ℹ️ How to interpret
Sizes are minified + brotli, measured with esbuild.
Package totals are computed as root size + marginal subpath costs.
Subpath marginal cost = (root + subpath bundled together) − root alone.
| Icon | Meaning |
|---|---|
| ✅ | No change |
| 🔺 | Increased ≤ 10% |
| 🔴 | Increased > 10% |
| 🔽 | Decreased |
| 🆕 | New (no baseline) |
Run pnpm size locally to check current sizes.
Re-exports don't work — tsdown unbundle mode resolves re-export chains to source modules, bypassing leaf define files and their customElements.define() calls entirely. Instead, composite define files now import element classes directly from source and call customElements.define() inline with guards to prevent double registration. Leaf define files are reverted to their original state (import + define only) and still work for direct imports. Closes #688 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
closed in #703 |
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
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
Fixes two bugs introduced by PR #635 (provider/container split) that both manifest as
StoreError: NO_TARGET:tsdown unbundle mode strips side-effect imports — bare
import './slider-fill'in composite define files gets tree-shaken away, so sub-elementcustomElements.define()calls never run. Fixed by inlining registrations directly in composite files withcustomElements.get()guards.@lit/contextupgrade order — ES import hoisting caused<media-container>to upgrade before<video-player>, so the consumer'scontext-requestevent fired before the provider existed. Fixed by extractingMediaContainerElementclass to a separate file (no auto-registration) and registering container after player.Also removes the
PlayerMixinmigration doc (not needed since the site isn't public yet) and fixes the playback-rate-button demo's missing<media-container>.Note: A third related bug (shared slider sub-elements across multiple slider types on the same page) is documented in #688 but not fixed in this PR.
Changes
Upgrade order fix:
define/media/container-element.ts— class-only, no registrationdefine/media/container.ts— simplified, imports from element filedefine/video/player.ts,define/audio/player.ts,define/background/player.ts— import fromcontainer-element, register player first, then container with guardInlined sub-element registrations:
define/ui/slider.ts,time-slider.ts,volume-slider.ts— inlinecustomElements.define()for slider sub-elementsdefine/ui/time.ts— inline registration for time-group, time-separatordefine/ui/controls.ts— inline registration for controls-groupslider-fill.ts,slider-thumb.ts, etc.) reverted to original stateDocs cleanup:
reference/player-mixin.mdxdocs.config.tsplayback-rate-buttondemo HTML (added<media-container>)Test plan
pnpm -F @videojs/html build— clean buildcustomElements.define()calls present for all sub-elementspnpm -F @videojs/html test— all tests passpnpm typecheck— no new errorsdata-*attributes and ARIA (tree-shaking fix verified)🤖 Generated with Claude Code