Refactor shady-render not to do "window" check as side-effect

[ryami333]

Fixes this issue. In short, this library can not be used in SSR frameworks like NextJS because there is a side-effect in shady-render which reads from the window variable, which would be undefined in an SSR context. This is a small refactor to allow users to do the following without a type error:

import { html, render } from "lit-html";

/* detect SSR: */
if (typeof window !== "undefined") {
  render(
    html`<h1>Hello World</h1>`,
    document.querySelector("[data-app]"))
  );
}

[christophediprima]

Hello there! What is the state of this one? This really needs to be fixed for SRR to work!

[ryami333]

I agree, in fact I think it's even more critical than that. lit-html can't even be imported in an SSR project right now. Which, by extension, means that any web components created with lit-html can't be either. This is a really straightforward patch and I hope the maintainers check it out soon.

[justinfagnani]

lit-html can't even be imported in an SSR project right now. Which, by extension, means that any web components created with lit-html can't be either. This is a really straightforward patch and I hope the maintainers check it out soon.

I've checked it out, but the problem is that this doesn't cover everything we'd need to do to make our libraries work in systems that run the code, but don't provide a DOM environment, and it It seems to me like there's a problem with the SSR library if it's running client-side JavaScript and not providing the minimal environment expected.

Is there not a way to customize the environment in Next.js to provide some global definitions?

[ryami333]

If you have the vision and motivation to make rendering work in a Node context (or anywhere that window is not defined) then I think that's a goal worthy of your project's Roadmap. This PR doesn't conflict with that goal, but it does get you part of the way there, with zero trade-offs. To simplify:

lit-html	Browser (importable + invokable)	Node (importable)	Node (invokable)
Before PR	✅	❌	❌
After PR	✅	✅	❌

---

there's a problem with the SSR library if it's running client-side JavaScript

That's my entire point, it's not running the code. By which I mean… Next is not trying to do any rendering with it (because of the typeof window !== "undefined" check), it's simply including the modules. And it's not a problem unique to NextJS or any "SSR Library", it would be a problem in any Node context.

[justinfagnani]

By importing a library, Next.js is by definition running code. JavaScript modules have top-level statements that run when the module is imported.

The problem I have with this approach is that it places the burden of interoperability on client-side modules that are not even intended to be used server-side at the moment. It only fixes one reference to window and doesn't handle other cases that occur in our closely related libraries like lit-element. We don't have any tests that ensure this module is importable in Node, and it would be easy to introduce a regression that isn't noticed until you get a new version.

There is a lot of client-side code out there that one could reasonably want to import from a Next.js project. There must be a more generic and scalable way to handle these situations.

It looks like this is their intended escape hatch: https://nextjs.org/docs/advanced-features/dynamic-import

import dynamic from 'next/dynamic';

const ClientComponent = dynamic(() => import('../components/component-that-uses-lit-html.js'));

function App() {
  return (
    <div>
      <ClientComponent />
    </div>
  );
}

Then ClientComponent can use any libraries that access DOM APIs at the top-level.

[ryami333]

By importing a library, Next.js is by definition running code

Yes, I'm well aware of this – but now you're using semantics to try and justify your point, and it's a strawman argument. The PR description already mentions that the issue in question is that the module has window dependent "side effects". I'd like to point out once again that this is not a NextJS-specific issue, but you're trying to "solve" the problem on my behalf with NextJS-specific workarounds, which I've already done. My problem though, is that I've actually published an internal web-component library, rather than, say, a React component library because of framework/platform agnosticism.

And yes, after inspecting other parts of this library (and lit-element) I did see that there are other window-dependent side-effects, but not many. I could of course go and fix those, but I thought I had better wait to see if there was any appetite from the maintainers to have this resolved for them though. And with regards to tests - well it would be very simple to create a test that simply synchronously imports this library in Node and expects no runtime errors. If I made those changes and included such a test, would this have a possibility of being merged?

[justinfagnani]

@ryami333 sorry, I didn't mean to leave this hanging so long. We're about to release lit-html 2.0, so I'd rather see any fixes related to Node go there. With the new monorepo, we have several tests running in Node already and it's not a big change to add a new one to ensure that modules remain importable there.