fix: remove media container observers on disconnect

[mihar-22]

As discussed in #1057.

Apologies, not sure why it's auto-formatting the innerHTML template string, am I missing an extension or something?

To answer a few questions from the other PR:

valid points though the constructor is called on an upgrade and most of the time that is on connected.

Generally true, but not when the element is created in-memory (document.createElement or via template) and not yet attached to the DOM. Quite common practice in modern frameworks (e.g., Vue/Solid/Svelte).

I believe we put it there because it is just setup once but it could be an oversight. it doesn't cause any serious issues but a new PR putting it in connectedCallback seems good to me. last I checked the cleanup logic is not absolutely required because the browser cleans that up automatically when the element is garbage collected.

That is true but it's still best practice to clean up on disconnect. Element may be held in memory as it's being moved in the DOM, might be offloaded in a scroll container as it goes out of view, detached by a framework as a performance optimization, and many other reasons.

[vercel]

@mihar-22 is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 15 lines in your changes missing coverage. Please review.

Project coverage is 77.49%. Comparing base (3ea80df) to head (3d61c04).
Report is 148 commits behind head on main.

Files with missing lines	Patch %	Lines
src/js/media-container.ts	85.71%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
- Coverage   78.55%   77.49%   -1.07%     
==========================================
  Files          59       51       -8     
  Lines       11080    12321    +1241     
  Branches        0      722     +722     
==========================================
+ Hits         8704     9548     +844     
- Misses       2376     2745     +369     
- Partials        0       28      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[luwes]

Apologies, not sure why it's auto-formatting the innerHTML template string, am I missing an extension or something?

no worries about that, I think I manually corrected that last time. Prettier is making that quite ugly :)

Generally true, but not when the element is created in-memory (document.createElement or via template) and not yet attached to the DOM. Quite common practice in modern frameworks (e.g., Vue/Solid/Svelte).

that's not correct, yes it is possible but it requires a manual https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/upgrade for that to happen. by default the upgrade is only done on connecting to the DOM, and the constructor will also be called at that time.

LGTM! I'll leave it for 1 other person to review

[cjpillsbury]

It's been awhile since thinking about these contortionist moves 😅. I can't recall whether or not one of the motivations for doing this in the constructor was to handle the possibility of doing a "first round" computation earlier than connectedCallback/DOM attachment (given the async crud) to reduce chances of CLS. Overall this approach makes sense to me and we can always re-solve the first pass case with this refactor if needed. AKA LGMT.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
media-chrome	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 7:09pm
media-chrome-demo-nextjs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 7:09pm
media-chrome-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 7:09pm

[mihar-22]

that's not correct, yes it is possible but it requires a manual https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/upgrade for that to happen. by default the upgrade is only done on connecting to the DOM, and the constructor will also be called at that time.

Sorry I wasn't clear. I was referring to the constructor and connectedCallback timing after the initial upgrade. It's all cursed voodoo magic anyway. You can see below that it can be constructed without being connected to the DOM in-memory.

Calling upgrade:

Simply appending to DOM: