solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
31.64k stars 887 forks source link

[Astro] `createResource` results get mixed up when rendering server only components with Suspense #2131

Open PeterDraex opened 2 months ago

PeterDraex commented 2 months ago

Describe the bug

resource.data() is returning data from another resource, in another component

Your Example Website or App

https://stackblitz.com/edit/github-pqy8j7-y8z78u?file=src%2Fcomponents%2FPage.tsx

Steps to Reproduce the Bug or Issue

  1. Open Page.tsx
  2. See that pageQuery resolves to 'pageQuery API response' in it's fetcher function, but when you check pageQuery.data(), it contains "Component A API response"

Expected behavior

This text is displayed

pageQuery.data: "pageQuery API response"

Screenshots or Videos

No response

Platform

Windows, Chrome 123

Additional context

@birkskyum:

It appear to happen because the data from the CompA resource isn't used anywhere, and then it ends up being picked up inside Page instead of the resource defined later there...

ryansolid commented 2 months ago

The setup for this is really odd. In that Astro seems to SSR Solid twice at least in dev and that is causing a whole bunch of chaos (not to mention sort of terrible for performance). I'm going to try to comment out the Astro code and see if there is any issue on Solid's side.

PeterDraex commented 2 months ago

Astro seems to SSR Solid twice

Unfortunately, that's by design according to https://github.com/withastro/astro/issues/8368

Unfortunately this is because we can't detect if a given function is a Solid component until we run it. Our React and Preact integrations do the same thing.

ryansolid commented 2 months ago

Ok positive is I don't think this is the cause even if it causes some weirdness in terms of multiple copies of resources. I will continue to investigate.

ryansolid commented 2 months ago

Ok I see the problem I just need to decide where the fix goes. There is an optimization it looks like someone put in to Solid-Astro integration to generate the non-hydration markup when you don't add client directives to your Solid components. NoHydration doesn't increment component IDs and parallel Suspense is resetting at the same id making those resources have identical IDs.

I admit I wasn't expecting this optimization. But I think this could happen in Solid's own Island implementation so I guess I need to make sure Suspense generates its own unique IDs. It will add depth but I'm not sure how else to avoid it.

PeterDraex commented 3 weeks ago

Hi, can I ask if this will be fixed in foreseeable future? Or is there any other way to avoid sending serialized data to the browser?

ryansolid commented 4 days ago

It's tricky to fix. I'd kinda rather see the Astro adapter drop the optimization in the short term rather than try to introduce new alternative functionality into Solid. I'm doing a release today and timeboxing it. I've been looking at this for the last hour if I can solve it in the next it goes in it, otherwise it will take some time.

Part of the challenge is reducing it to the minimal actually correct example because like data.loading doesn't Suspend and if the second boundary suspends there is no problem either. So I'm not sure what the bug is I can just see the symptoms of it. LIke in the reproduction no one is reading the data and triggering Suspense except the page, which is highly unusual. And it only happens when the Islands aren't hydrated. Any fix would need to not break hydration either.

ryansolid commented 4 days ago

Oh I guess I should mention there is a no scripts option in our render function that I think could be turned on in the no hydration case like yours but that isn't what is causing this issue and it would need to be exposed or set by the Astro renderer.

The problem here is id collision during SSR. I can fix it by removing an optimization to reduce id depth with no-hydration.. basically reducing the efficiency of the no-hydration optimization but I think it will work. I will need to test it though in other SSR environments.

ryansolid commented 4 days ago

It will work but it would break SolidStart so I can't make this a patch release. It changes the SSR behavior of libraries depending on <NoHydrate>. They will have to account for the change in ID generation. So I can't fix this now.

I think that if you are in the no hydration scenario fetch your data outside of Solid and pass it through is my recommendation. There is nothing interactive so the data fetching doesn't need to live in Solid. All the other cases seem to work.