[astro-ci] Lit element SSR failure

[bholmesdev]

Describe the bug

We are receiving a window is not defined error when using our Lit Element SSR renderer. This is likely a problem with Vite respecting our shims for browser globals like window.

Reproduction

(see logs)

System Info

System:
    OS: macOS 12.0.1
    CPU: (8) arm64 Apple M1
    Memory: 304.47 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.2 - /usr/local/bin/node
    Yarn: 1.22.18 - /usr/local/bin/yarn
    npm: 8.1.2 - /usr/local/bin/npm
  Browsers:
    Chrome: 103.0.5060.134
    Firefox: 102.0.1
    Safari: 15.1

Used Package Manager

pnpm

Logs

1. Run pnpm build and pnpm link --global from ~/repos/vite/packages/vite
2. Run pnpm link --global vite from ~/repos/astro/packages/astro
3. Run pnpm dev (or build) from ~/repos/astro
4. Run a known failing test, like pnpm test:match "Lit integration in SSR
5. Hit the following error:

ReferenceError: window is not defined
      at file:///Users/benholmes/Repositories/astro/node_modules/.pnpm/@lit+reactive-element@1.3.3/node_modules/@lit/reactive-element/css-tag.js:6:9
      at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
      at async Promise.all (index 0)
      at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
      at async Object.loadTestAdapterApp (file:///Users/benholmes/Repositories/astro/packages/astro/test/test-utils.js:155:36)
      at async fetchHTML (file:///Users/benholmes/Repositories/astro/packages/astro/test/ssr-lit.test.js:22:15)
      at async Context.<anonymous> (file:///Users/benholmes/Repositories/astro/packages/astro/test/ssr-lit.test.js:31:16)

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[matthewp]

I think this is likely caused by this change: https://github.com/vitejs/vite/pull/9146/files

We have specific module ids we externalize: https://github.com/withastro/astro/blob/main/packages/integrations/lit/src/index.ts#L16-L22

[patak-dev]

@matthewp #9146 was released in 3.0.1. It is a possibility then that we broke Astro CI before the latest release then.
Sorry for this change, but it is correcting how ssr.noExternal and ssr.external works to align it with Vite 2. The idea is that adding a dependency to noExternal should bundle every entry point. This has always worked at the package name level but 3.0.0 was released with regression.
So:

			external: [
				'lit-element/lit-element.js',
				'@lit-labs/ssr/lib/install-global-dom-shim.js',
				'@lit-labs/ssr/lib/render-lit-html.js',
				'@lit-labs/ssr/lib/lit-element-renderer.js',
				'@astrojs/lit/server.js',
			],

Should be:

			external: [
				'lit-element',
				'@lit-labs/ssr',
				'@astrojs/lit',
			],

But I don't understand why these needs to be in ssr.external, as they should be already external by default, no? Expect if you have ssr.noExternal: true or some regex covering all these package names.

[matthewp]

Maybe that was the intended behavior but Vite 2.0 let you externalize specific modules and not just entire packages. We've been using external this way as long as we've been using Vite: https://github.com/withastro/astro/blob/b7a4542103c1193e125ec0c069749d4bf98f223d/packages/renderers/renderer-lit/index.js#L4

I think your solution might work in this case, I'll try it. I wonder why this is the intended behavior though. Packages can have multiple entrypoints and some of them might need to be externalized and some others not.

[patak-dev]

cc @brillout @bluwy @benmccann @dominikg, sorry for pinging you all but we better correctly define and specify how ssr.noExternal, ssr.external should work.
For noExternal, we already saw that in Vite 2, it applied to the package name. In Vite 3 we launched with noExternal working at the entry instead and reverted that because Svelte wasn't working with this change.
I find it odd that noExternal would work at the package name level and external at the entry level but this is a single line of code change. Could you review how this should work so vite-plugin-ssr, SvelteKit, and Astro are happy with the options?
I'm thinking that we may want to review the naming in Vite 4 now that everything is external by default, so we may get another opportunity to review. But we should finalize the version that we will use in Vite 3 as soon as possible.

[benmccann]

It sounds like the Vite 2 behavior was packages for noExternal and modules for external? Is that right? It sounds like that worked well-enough for everyone for these options even if it's weird they're different

[bluwy]

IIUC ssr.noExternal is working fine now, but we're deciding if ssr.external should work with package name, entries/modules, or both? I think having it work with both make sense to me. external can be finer-grain if a specific file needs to be externalized.

[patak-dev]

@matthewp looks like Vite v2 indeed allowed to define both specific entries in ssr.external and package names (and in that case every entry is external). I'll send a PR soon to correct this regression, so your PR to Astro shouldn't be needed once 3.0.3 is out. Sorry for these changes (I know they are borderline breaking changes for 3.0, but this wasn't an intended change).

I'm curious though about why you needed the ssr.external in Vite 3. Are you setting noExternal: true? I think they should be externalized by default and you could remove the config. If that doesn't work maybe there is a bug in the new externalization logic.

[brillout]

Sounds good to me.

Although I also wonder why ssr.external is needed in the first place since that's Vite 3's default behavior. I don't even see a use case for ssr.external. Even with ssr.noExternal: true (because AFAICT ssr.noExternal: true is used for bundling the whole serer code, so using ssr.external would contradict that).

If it'd be up to me I'd remove ssr.external altogether, but I may miss a use case here.

[patak-dev]

It could be used to make a dependency noExternal but externalize one of its entries. I don't think there will be many cases where that is useful though.
In Vite 3, external takes precedence over noExternal: true, so it could also be used to only flip the default and chose what you would like to externalize. But again, I don't think there will be many use cases for this.
I'm curious about why Astro was failing when the ssr.external wasn't hitting the right entries. I would expect that entries to be external by default, so as I said in the last message, I think we have a bug.

[matthewp]

We can probably revisit this now. In the past it was because deps were optimized in SSR (are they never any more?) which led to the possibility of multiple instances of a package existing and instanceof breaking. So we were forcing packages to be noExternal if they had dependencies that would have been optimized.

The other (but related) reason (and why this specific case breaks) is because this code:

import 'a';
import 'b';

if a contains side-effects that b depends on it has to run before b. If b is external then so must a be, otherwise it will break.

We apply noExternal to any Astro-ecosystem packages because we allow you to ship packages with unbuilt .astro files (oops, maybe a mistake), so it's possible to run into this scenario.

---

tldr; we (ab)use noExternal and external heavily. maybe we should calm down.

[patak-dev]

Ok, I see. If you want to have .astro files in your SSR deps, then yes, you would need to have these dependencies as noExternal (even without deps optimization enabled, that by default is off in v3 right now). So I see why you have the need for the ssr.external keyboard.
I think @dominikg and @bluwy are doing something similar with .svelte files, so I don't think you should change how you are using noExternal.

[brillout]

I would expect that entries to be external by default, so as I said in the last message, I think we have a bug.

Same suspicion.

As for ssr.external yes I agree that in theory it may be useful while I also fail to see a concrete use case for it.

[patak-dev]

@brillout I think that @matthewp gave a concrete use case in his prev message, and hinted that there may not be a bug. They are adding deps to noExternal because they may have .astro files that needs to be processed, and then they want some of the entries in these to be external. The reported case doesn't look like this, but at least in theory this is a valid use case

[brillout]

Ok, I think I understand now: a package needs partial externalization if some files are source files (= noExternal) while other files need to be deduped (= external).

[matthewp]

It would be nice if you could configure this scenario with some sort of config. I could see some config like sourceExtensions: ['.astro', '.vue'] which would noExternal anything imported that way. Then you wouldn't need to use external or noExternal very often.