withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.8k stars 2.49k forks source link

Signals inside array/objects doesnt serialize correctly #7864

Closed pudymody closed 2 months ago

pudymody commented 1 year ago

What version of astro are you using?

2.9.6

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

None

What package manager are you using?

npm

What operating system are you using?

Linux

What browser are you using?

Firefox 115.0.3

Describe the Bug

Hi, im new to this Astro-(p)react world, so i dont know if im doing things the right way, but i think i found bug when using signals whitin arrays/objects.

Tweaking the example to pass an array of signals as a prop, doesnt render anything and actions doesnt work either.

const countArray = [
    signal(0),
    signal(0)
]
<CounterArray count={countArray} client:visible>
<h1>Hello, from array!</h1>
</CounterArray>
<div class="counter">
  <button onClick={subtract0}>-</button>
  <pre>{count[0].value}</pre>
  <button onClick={add0}>+</button>
</div>

Here i have used value because it gave an error about cyclic dependencies

At first i thought the error was here as it only checks for shallow signals, but i havent investigated any further nor deep enough.

https://github.com/withastro/astro/blob/d2b6dabfb37540774fb70d2d58aa7ba2f335e2dc/packages/integrations/preact/src/signals.ts#L32

What's the expected result?

Signals should be serialized independant whether they are inside an array/object or not.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-488db8?file=src%2Fpages%2Findex.astro

Participation

natemoo-re commented 1 year ago

Thanks for opening an issue! I think we might be serializing these correctly, but the Preact integration isn't rehydrating the signals properly.

If you want to dig into this and open a PR, we'd be happy to take a look! The serialization logic for Preact should be mostly in the signals.ts file.

jocchong commented 1 year ago

Looking at signals.ts as @natemoo-re suggested, we skip serializing signals nested in arrays (and objects) entirely because isSignal() returns false, thus they are never added to the data-preact-signals attribute.

If we wanted to support arbitrary-depth nesting of signals, calling serializeSignals() recursively on arrays/objects seems like the correct path forward.

natemoo-re commented 1 year ago

@jocchong you might want to hold off on a PR until https://github.com/withastro/astro/pull/8004 is merged since we'll be updating our serialization approach for 3.0. I think the signal logic will remain the same, but it's possible there will be a cleaner way to do this using seroval

ariadne-github commented 10 months ago

@natemoo-re any news on props serialization? I can see that #8004 has been closed. I was wondering if someone is working on it, so that we can have components which use large data without the props bloating the html