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

onMount not called in nested hydrated Solid.js components #4343

Closed
1 task
DevHusariaSolutions opened this issue Aug 16, 2022 · 12 comments · Fixed by #4891
Closed
1 task

onMount not called in nested hydrated Solid.js components #4343

DevHusariaSolutions opened this issue Aug 16, 2022 · 12 comments · Fixed by #4891
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)

Comments

@DevHusariaSolutions
Copy link

DevHusariaSolutions commented Aug 16, 2022

What version of astro are you using?

1.0.5

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

What package manager are you using?

npm

What operating system are you using?

Windows

Describe the Bug

I have code, which should update signal state and render app using component.

export function AppLayout(props: { children: JSXElement }) {
  const c = children(() => props.children)
  const [show, setShow] = createSignal(false)

  onMount(() =>
    setTimeout(() => {
      setShow(true)
      console.log('🚀 ~ file: AppLayout.tsx ~ line 14 ~ AppLayout ~ onMount', show()) // it shows true
    }, 300)
  )

  return (
    <Show when={show()}>
      <div id="root">
        <FallingStarsWrapper />
        <Navbar />
        <div id="app-content">
          {c()}
          <Footer />
        </div>
      </div>
    </Show>
  )
}
  1. onMount doesn't fire
  2. switching to another hook (createRenderEffect) does the job, but once signal is update - no change in UI occurs

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-sdeict?file=src/components/Counter.tsx

Participation

  • I am willing to submit a pull request for this issue.
@DevHusariaSolutions DevHusariaSolutions changed the title Solid-JS: **onMount** is not firing, reactivity seems broken Solid-JS: onMount is not firing, reactivity seems broken Aug 16, 2022
@bluwy bluwy added pkg: solid Related to Solid (scope) - P2: has workaround Bug, but has workaround (priority) labels Aug 16, 2022
@bluwy
Copy link
Member

bluwy commented Aug 16, 2022

I can reproduce this, though it can be workaround by wrapping the <Show/> with a div. Also onMount seems to be called if the signal starts with true first, then set to false. This doesn't seem to happen in the solidjs playground so I think something in Astro is messing with it.

@DevHusariaSolutions
Copy link
Author

@bluwy such workaround doesn't work for us. Also onMount doesn't work for us at all.

@natemoo-re natemoo-re changed the title Solid-JS: onMount is not firing, reactivity seems broken Conditionally hidden children do not hydrate when using client:visible Aug 16, 2022
@natemoo-re
Copy link
Member

natemoo-re commented Aug 16, 2022

Hey @DevHusariaSolutions! The easiest workaround here is to switch from client:visible => client:idle.

https://stackblitz.com/edit/github-sdeict-8xbpkg?file=src%2Fpages%2Findex.astro

Why is this happening? client:visible uses and IntersectionObserver under the hood, but since the astro-island element uses display: contents, we need to observe any child elements for visibility changes. In this case, your fallback content doesn't include any elements, only text, which cannot be observed for visibility changes.

Another workaround would be to wrap you child content in any element so the client:visible is triggered correctly.

-    <Show when={show()} fallback="Not working">
+    <Show when={show()} fallback={<p>Not working</p>}>

@natemoo-re natemoo-re removed the pkg: solid Related to Solid (scope) label Aug 16, 2022
@natemoo-re natemoo-re changed the title Conditionally hidden children do not hydrate when using client:visible Conditionally hidden children do not hydrate when using client:visible and fallback content is only a Text Node Aug 16, 2022
@natemoo-re
Copy link
Member

Removed the framework-solid tag as this is not Solid specific.

I think we could inject an empty <span> if client:visible is used but the children do not contain any visible elements (only text and template nodes, as is the case here).

@DevHusariaSolutions
Copy link
Author

DevHusariaSolutions commented Aug 17, 2022

@natemoo-re thanks for Your response. In sandbox it works.
But once I change it in my code - it doesn't (onMount not firing at all).
No matter I use client:idle or client:visible.

I found out what's the issue.
When I use Solid components with client:visible (or any client directive) inside Layout astro component (which uses Solid component AppLayout), then onMount in AppLayout is not firing o.O

Check my edited StackBlitz https://stackblitz.com/edit/github-sdeict?file=src%2Fpages%2Findex.astro

@DevHusariaSolutions
Copy link
Author

@natemoo-re please restore framework-solid flag, because I guess it's too early to decide it's not related to framework

@DevHusariaSolutions
Copy link
Author

@bluwy pls remove flag p2-has-workaround, because it hasn't

@bluwy bluwy added - P4: important Violate documented behavior or significantly impacts performance (priority) s2-medium and removed - P2: has workaround Bug, but has workaround (priority) labels Aug 20, 2022
@FredKSchott FredKSchott added - P2: has workaround Bug, but has workaround (priority) - P3: minor bug An edge case that only affects very specific usage (priority) and removed - P4: important Violate documented behavior or significantly impacts performance (priority) - P2: has workaround Bug, but has workaround (priority) labels Aug 22, 2022
@FredKSchott FredKSchott changed the title Conditionally hidden children do not hydrate when using client:visible and fallback content is only a Text Node onMount not called in nested hydrated Solid.js components Aug 22, 2022
@FredKSchott FredKSchott added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Aug 22, 2022
@FredKSchott
Copy link
Member

Thanks for the updated Stackblitz! I can reproduce in your stackblitz.

If you have any interest in jumping in to help with a fix, let us know! Otherwise we'll get to this as soon as we can.

@DevHusariaSolutions
Copy link
Author

DevHusariaSolutions commented Aug 22, 2022

We are full of work now. Just because of this bug we are stuck with some app refactor, and we need to use another package.
But Astro seems be better, I like it just because we switched from Svelte, which had same nice composition of logic and UI.
Hopefully You can make Solid work on Astro in next weeks, in free time I can give some look for code too.

@bluwy

This comment was marked as outdated.

@bluwy

This comment was marked as outdated.

@bluwy
Copy link
Member

bluwy commented Sep 19, 2022

Ignore my comment above again 😺 Looks like the issue is that the astro island script is added in the slot rather than the first wrapping client:* component. Seems to be a variant of #4288

New repro: https://stackblitz.com/edit/github-r17dcx?file=src%2Fpages%2Findex.astro&on=stackblitz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants