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

Astro Island Not Passing Attributes to Sub-Components, Rendering Twice, Issues With Lit #5304

Closed
1 task
davidbwaters opened this issue Nov 5, 2022 · 18 comments
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: lit Related to Lit (scope)

Comments

@davidbwaters
Copy link

What version of astro are you using?

1.6.3

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

CodeSandbox

Describe the Bug

When using client directives in astro, it's not rendering attributes. On codesandbox, it's not even passing them when importing as a script. It's also rendering components twice when importing through a normal script tag. I've experienced this a little bit when running locally rarely. Also, when defined in typescript, components aren't hydrating. Using JS it works. Just check out the codesandbox example and you'll see what i mean.

Link to Minimal Reproducible Example

https://codesandbox.io/s/inspiring-agnesi-8h0z1d?file=/src/pages/index.astro

Participation

  • I am willing to submit a pull request for this issue.
@matthewp
Copy link
Contributor

If you have a reactive property it sets the property rather than the attribute. This is because some properties have to be rendered as props. What problem is caused by not setting the attribute?

@matthewp matthewp added the needs response Issue needs response from OP label Nov 16, 2022
@davidbwaters
Copy link
Author

davidbwaters commented Nov 18, 2022

I guess it's just not hydrating when used as an astro component and it's sometimes rendering twice when importing as a script. Check the codesandbox demo and look at index2. i've seen the rendering twice thing in my personal usage also.

CleanShot 2022-11-18 at 00 17 40@2x

@matthewp matthewp self-assigned this Nov 18, 2022
@matthewp matthewp added bb:investigate and removed needs response Issue needs response from OP labels Nov 18, 2022
@matthewp matthewp added the - P3: minor bug An edge case that only affects very specific usage (priority) label Dec 6, 2022
@matthewp matthewp removed their assignment Dec 8, 2022
@matthewp
Copy link
Contributor

Same issue reported here: #6342

Did a little investigation and I think this is a Lit bug. It happens simply by having:

<script>
  import "lit";
</script>

I think what is happening is there is a race condition here. I think that perhaps Lit is rendering before the HTML children have been appended, so it doesn't see that they are there. That is my theory at least. It does seem unrelated to anything Astro is doing though.

Going to ping @e111077 if he wants to take a look, the example in #6342 is a pretty good reduction, but you can remove the client component and just have a import "lit" and see the same thing.

@e111077
Copy link
Contributor

e111077 commented Feb 23, 2023

This is a composition issue on the Astro side. When you include the Astro Lit integration, you should be inserting import 'lit/experimental-hydrate-support.js' before the Lit library is loaded anywhere.

Unfortunately I do not know how to do this in the Lit integration. But there are 2 workarounds:

  1. Suggested: Use the JSX client:only="lit" syntax we just shipped and update to @astro/[email protected] (example)
  2. Add the hydration script to <head> in your main layout (example) though not sure how Astro handles library loading order

In Astro, it's expected that framework components should be loaded this way client:only (please confirm @matthewp if this is a safe assertion to make). Option 1 should not affect any other non-Lit custom elements (you should still be able to load non-lit elements like in your example), so it's a valid migration.

Hopefully there's a way an integration can inject a script to the top of the page, because in that case, loading the lit integration, it should just load the hydration support shim

UPDATE: I updated the examples to use the lit client:only directive on a vanilla WC and it also seems to work in the general case

@matthewp
Copy link
Contributor

Ah, we do have the hydration script, perhaps that is not being added correctly? I'll look into this tomorrow, thanks for the research @e111077

@e111077
Copy link
Contributor

e111077 commented Feb 24, 2023

It might not be loaded early enough, and inline scripts may be injecting before it, or alternatively, it could be how HMR is doing things in relation to loading a Lit element without the client:only JSX route

@RogierdeRuijter
Copy link

As mentioned above by @e111077 inline scripts that query the document doesn't work with the client:only="lit", because the injection happens to late. I believe we should be able to use client:visible and other island architecture attributes for lit components. It feels like a bug that should be addressed.

@e111077
Copy link
Contributor

e111077 commented Mar 15, 2023

clarification, the inline scripts i'm talking about – they may be loading the lit module before the lit experimental-hydrate module is loaded. This will cause Lit to skip the hydration step altogether, causing the double rendering. A workaround for now can be to simply import the hydration script, it won't be any extra bytes over the wire as long as you eventually have an astro lit component (which you will because that's what's ultimately causing this failure)

@RogierdeRuijter
Copy link

RogierdeRuijter commented Mar 16, 2023

When running npm run build && npm run preview in this stackblitz. The double rendering happens. It seems to to be ok when running npm run dev but with a full build and preview the problems happens. Here is a sandbox. https://stackblitz.com/edit/node-uy3xxu?file=src%2Fpages%2Findex.astro,src%2Fcomponents%2Fcalc-add.js

@Princesseuh Princesseuh added the pkg: lit Related to Lit (scope) label Jul 18, 2023
@tjkohli
Copy link

tjkohli commented Aug 9, 2023

+1 we are also experiencing this issue in a component on a live website. Following 👀

@JosefJezek
Copy link

any progress?

@marcmalerei
Copy link

marcmalerei commented Sep 20, 2023

Hi, i have the same problem using astro "3.0.12" and @astrojs/lit "3.0.0". Using the directive client:only="lit" is not an option in my scenario as i need the static html as well.

I also get the following console error
image

@marcmalerei
Copy link

marcmalerei commented Sep 22, 2023

I think i found one of the problems:

If i have a component (A) which is included in the astro page with a directive "client:visible" i will have duplicate output of the component (B), which is nested in component (A), as long as i do the import of component (B) before a instance of (A) exists.

This does not work:

Component A

import { LitElement } from "lit";
import { customElement } from "lit/decorators.js";
import "./component-b";

@customElement("component-a")
export class ComponentA extends LitElement {}

Component B

import { LitElement } from "lit";
import { customElement } from "lit/decorators.js";

@customElement("component-b")
export class ComponentB extends LitElement {}

This does work:

Doing a dynamic import in the connectedCallback does solve the problem.
Component A

import { LitElement } from "lit";
import { customElement } from "lit/decorators.js";


@customElement("component-a")
export class ComponentA extends LitElement {
  connectedCallback(){
    import('./component-b');
  }
}

Component B

import { LitElement } from "lit";
import { customElement } from "lit/decorators.js";

@customElement("component-b")
export class ComponentB extends LitElement {}

I hope that helps somehow.

@ditorodev
Copy link

Im using React, and this has happened to 2 of my users consistently on a Chrome browser. Thing is I cant really replicate it locally and its a really reallyyyy weird bug

@lilnasy
Copy link
Contributor

lilnasy commented Dec 4, 2023

Some of the issues pointed out in the original description seem to have been fixed.

Facts as of today:

  • The component needs to be imported manually in a script tag for it to be duplicated.: <script>import '../components/my-counter.js';</script>
  • There must also be a second unrelated lit component used on the same page. It must be imported in the frontmatter and used as an island, but it does not need to be given a client directive: <CalcAdd/>
  • It affects the dev mode as well, but only if you navigate to another page and then back.

The "not passing props to attributes" seems to have been fixed at some point: index-2 works now, it did not in [email protected].

Since importing manually is not something we document, my view is that the remaining issue would be a corner case that can be avoided by importing MyCounter as a regular component instead.

@lilnasy lilnasy added - P2: has workaround Bug, but has workaround (priority) and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Dec 4, 2023
@bencipher
Copy link

I am also facing the issue, data gets saved twice to my db as a result of this. I followed the P2: has a workaround link but I didn't quite get it as I saw bunch of issue on a page.

image

@matthewp
Copy link
Contributor

matthewp commented Apr 2, 2024

Agree with @lilnasy in that many of these issues have been fixed. Going to close as it's difficult to try and fix the remaining issues without a clear example of them. I would suggest that anyone that still has issues related to these in the Lit renderer please create a new issue specifically for them.

@karlludwigweise
Copy link

I've filed a new issue for this including a reproducible example: #12198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) pkg: lit Related to Lit (scope)
Projects
None yet
Development

No branches or pull requests