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

[SSR] Add option to shim without window #3412

Closed
aomarks opened this issue Nov 1, 2022 · 0 comments · Fixed by #3431
Closed

[SSR] Add option to shim without window #3412

aomarks opened this issue Nov 1, 2022 · 0 comments · Fixed by #3431
Assignees

Comments

@aomarks
Copy link
Member

aomarks commented Nov 1, 2022

Many libraries check for the presence of window to decide if they are running in the browser or on the server. This can pose a problem when trying to run such a library alongside Lit SSR, because the Lit SSR DOM shim defines window, so libraries that would usually be compatible with Node get confused about where they're running and break.

We don't really need to define window in our DOM shim, because any supported global API (e.g. customElements.define) that a Lit component library needs to call for SSR can be referenced without the namespace, or using globalThis instead of window.

However, it is somewhat common to see e.g. window.addEventListener in connectedCallback, so removing window would break those cases. Ultimately, I think it's a better trade off to not define window, and just require those occasional lines to switch to globalThis. (We don't actually call connectedCallback or shim addEventListener currently, but we likely will soon).

In the immediate term, to avoid a breaking change, maybe we should make the non-window defining version opt-in. We could export another function from dom-shim.js called e.g. installDomShimWithoutWindow -- and then eventually make this the standard behavior.

(Side note: our implementation of isServer uses node export conditions instead of checking any globals, which I think would be a great direction for other libraries to move in, since it avoids this problem and also avoids any runtime check: https://github.com/lit/lit/blob/main/packages/lit-html/src/is-server.ts)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants