Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use globalThis i/o window (declarative shadow DOM) #671

Merged
merged 4 commits into from
Jul 13, 2023

Conversation

luwes
Copy link
Contributor

@luwes luwes commented Jun 18, 2023

I'm making some progress with a generic solution for SSR.
Got it working in a plain Node app and Cloudflare worker, working on Next.js now... (added https://wesc-nextjs.vercel.app/)
https://github.com/luwes/wesc

The worker is pretty cool, it can take any URL with Media Chrome components and add DSD to it
https://wesc.luwes.workers.dev/?url=https%3A%2F%2Fmedia-chrome.mux.dev%2Fexamples%2Fvanilla%2Fadvanced.html
https://wesc.luwes.workers.dev/?url=https://www.media-chrome.org/

There is a limit on CPU time for cloudflare workers so it might time out but I haven't seen it yet.
SSR'ing 3 Media Chrome players took like 35ms locally.


one of the things that came up is that some frameworks like Next.js recommend server checks with typeof window === 'undefined' https://stackoverflow.com/questions/49411796/how-do-i-detect-whether-i-am-on-server-on-client-in-next-js

so we don't want to be shimming window but globalThis with a DOM shim otherwise we are making the server / build look like a client environment.

@luwes luwes requested review from a team and heff as code owners June 18, 2023 16:17
@luwes luwes self-assigned this Jun 18, 2023
@vercel
Copy link

vercel bot commented Jun 18, 2023

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 Jul 13, 2023 7:30pm
media-chrome-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 13, 2023 7:30pm

@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

Merging #671 (e6a61b6) into main (93ff3cb) will increase coverage by 0.00%.
The diff coverage is 78.36%.

@@           Coverage Diff           @@
##             main     #671   +/-   ##
=======================================
  Coverage   78.44%   78.45%           
=======================================
  Files          43       43           
  Lines        8206     8205    -1     
=======================================
  Hits         6437     6437           
+ Misses       1769     1768    -1     
Impacted Files Coverage Δ
src/js/media-time-range.js 70.86% <27.27%> (ø)
src/js/utils/captions.js 68.60% <33.33%> (+0.10%) ⬆️
src/js/media-container.js 82.18% <36.36%> (ø)
src/js/media-live-button.js 68.46% <60.00%> (-0.72%) ⬇️
src/js/media-controller.js 74.66% <64.70%> (ø)
src/js/controller.js 63.34% <66.66%> (ø)
src/js/media-airplay-button.js 77.61% <75.00%> (ø)
src/js/media-cast-button.js 74.33% <75.00%> (ø)
src/js/media-fullscreen-button.js 77.34% <75.00%> (ø)
src/js/media-mute-button.js 78.26% <75.00%> (ø)
... and 22 more

@luwes
Copy link
Contributor Author

luwes commented Jul 1, 2023

Got a Next.js example running! https://wesc-nextjs.vercel.app/

In the new Next.js app router it's really minimal what you need to add because it supports async components

@luwes luwes changed the title fix: use globalThis i/o window fix: use globalThis i/o window (declarative shadow DOM) Jul 1, 2023
@heff
Copy link
Collaborator

heff commented Jul 6, 2023

so we don't want to be shimming window but globalThis with a DOM shim otherwise we are making the server / build look like a client environment.

Have you actually run into this problem? We're not setting window globally, just locally to the project, so any typeof window check shouldn't be affected by our local usage of a variable named window right?

globalThis is already "server safe", so it feels backwards to then create a "server safe shim" for it. It also doesn't have a consistent API across platforms, so how do you shim it? We're intentionally shimming the window API, not globalThis so
components expecting window functions don't blow up on the server.

@luwes
Copy link
Contributor Author

luwes commented Jul 7, 2023

@heff I haven't run into any issues but I think it would break that typeof window === 'undefined' check though.

I know the window export variable is local to Media Chrome but the DOM API's have to be shimmed on a global object right, either window or globalThis.

In the window scenario this will happen.

// This will be true if JSDOM or Linkedom shimmed window with equivalent API's.
const isShimmed = Object.keys(windowShim)
  .every(key => key in window);

// If we keep using window the `window` in the ternary else has to be shimmed with equivalent API's for SSG / SSR to work
export const Window = isServer && !isShimmed ? windowShim : window;

// => typeof window === 'undefined' won't be true anymore 

That's the thing, we're making the Node environment or Cloudflare Worker environment support all the browser API's we need for it to create the (shadow) DOM and then output it as HTML (DSD) => SSG / SSR web components

Look at view source of this page https://wesc-nextjs.vercel.app/
It has all the shadow DOM already in the page so the browser doesn't require the templates in the Javascript bundle anymore to render.

@heff
Copy link
Collaborator

heff commented Jul 11, 2023

but the DOM API's have to be shimmed on a global object right, either window or globalThis.

No? We've shimmed all the DOM APIs we've needed on windowShim. As long as we continue to import our internal version of window whenever a window API is needed, no shim has to go on a global object.

I'm not clear on if what you're doing here requires a bunch of additional window APIs though. I think I need you to walk me through this feature in general to get caught up.

Copy link
Collaborator

@cjpillsbury cjpillsbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The future is now.

@luwes luwes merged commit f620c1e into muxinc:main Jul 13, 2023
3 of 4 checks passed
@luwes luwes deleted the globalThis branch July 13, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants