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

Update lit-ssr dependency #6681

Merged
merged 13 commits into from
Mar 29, 2023
Merged

Update lit-ssr dependency #6681

merged 13 commits into from
Mar 29, 2023

Conversation

e111077
Copy link
Contributor

@e111077 e111077 commented Mar 27, 2023

fixes #6670

Changes

  • Updates lit-ssr dependency to match that of lit@^2.7.0's requirements
  • Update integration with new lit-ssr changes (no global window)

Testing

Usual test run – tests updated

Docs

Should not require behavior change

@changeset-bot
Copy link

changeset-bot bot commented Mar 27, 2023

🦋 Changeset detected

Latest commit: 4b9a03e

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: lit Related to Lit (scope) pkg: integration Related to any renderer integration (scope) labels Mar 27, 2023
packages/integrations/lit/server-shim.js Outdated Show resolved Hide resolved
packages/integrations/lit/server-shim.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) labels Mar 28, 2023
@@ -8,10 +8,6 @@ const test = testFactory({
// TODO: configure playwright to handle web component APIs
// https://github.com/microsoft/playwright/issues/14241
test.describe('Lit components', () => {
test.beforeAll(() => {
delete globalThis.window;
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer needs to shim the window?

Copy link
Contributor Author

@e111077 e111077 Mar 28, 2023

Choose a reason for hiding this comment

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

All of the shims needed for lit are baked into Lit's node build. I just pushed a commit though because we do need to actually re-shim only customElements.define and HTMLElement because our baked-in shims in ReactiveElement's node build is forgiving and tries to call anything anyone else has shimmed, but we have a small hack we are relying on so we re-shim that

Copy link
Contributor

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

This part of the readme should be updated

### Browser globals
The Lit integration's SSR works by adding a few browser global properties to the global environment. Some of the properties it adds includes `window`, `document`, and `location`.
These globals *can* interfere with other libraries that might use the existence of these variables to detect that they are running in the browser, when they are actually running in the server. This can cause bugs with these libraries.
Because of this, the Lit integration might not be compatible with these types of libraries. One thing that can help is changing the order of integrations when Lit is interfering with other integrations:
```diff
import { defineConfig } from 'astro/config';
import vue from '@astrojs/vue';
import lit from '@astrojs/lit';
export default defineConfig({
- integrations: [vue(), lit()]
+ integrations: [lit(), vue()]
});
```
The correct order might be different depending on the underlying cause of the problem. This is not guaranteed to fix every issue however, and some libraries cannot be used if you are using the Lit integration because of this.

.changeset/gold-windows-fly.md Outdated Show resolved Hide resolved
packages/integrations/lit/server-shim.js Outdated Show resolved Hide resolved
.changeset/gold-windows-fly.md Outdated Show resolved Hide resolved
@e111077
Copy link
Contributor Author

e111077 commented Mar 29, 2023

@matthewp would also like to note that we have to re-shim HTMLElement because Astro's DOM shim doesn't seem to do anything when you do

element.setAttribute('foo', 'bar')
element.setAttribute('baz', 'quux')
element.getAttribute('foo') // bar
element.attributes // ['foo', 'baz']

https://github.com/withastro/astro/blob/main/packages/webapi/src/lib/Element.ts#L25-L32

I wonder WYT about implementing that in Astro?

@e111077 e111077 marked this pull request as ready for review March 29, 2023 02:29
@e111077
Copy link
Contributor Author

e111077 commented Mar 29, 2023

You also might want to squash merge this one 🙈

@matthewp
Copy link
Contributor

Thanks! I'm not sure how we feel about having the DOM shim in long term. Sort of feel like the renderer should bring its own if needed. So I think what you are doing here is fine for now, thanks!

@matthewp
Copy link
Contributor

matthewp commented Mar 29, 2023

@augustjk is this ok with you now? can you give your 👍 if you think this is good to merge?

@augustjk
Copy link
Contributor

Looking good, thank you!!

@matthewp matthewp merged commit 4b07731 into withastro:main Mar 29, 2023
@astrobot-houston astrobot-houston mentioned this pull request Mar 30, 2023
davidenke added a commit to kulturverein-lochmuehle/website that referenced this pull request Mar 30, 2023
but waiting for next astro release to contain a fix for latest lit

withastro/astro#6670
withastro/astro#6681
davidenke pushed a commit to kulturverein-lochmuehle/website that referenced this pull request Oct 10, 2023
but waiting for next astro release to contain a fix for latest lit

withastro/astro#6670
withastro/astro#6681
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: lit Related to Lit (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lit Integration with client:load throws hydration exception
3 participants