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

Signals inside array/objects doesnt serialize correctly #7864

Closed
1 task done
pudymody opened this issue Jul 29, 2023 · 4 comments · Fixed by #11834
Closed
1 task done

Signals inside array/objects doesnt serialize correctly #7864

pudymody opened this issue Jul 29, 2023 · 4 comments · Fixed by #11834
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) help wanted Please help with this issue! pkg: preact Related to Preact (scope)

Comments

@pudymody
Copy link

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.

for (const [key, value] of Object.entries(props)) {

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

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jul 29, 2023
@natemoo-re natemoo-re added pkg: preact Related to Preact (scope) - P3: minor bug An edge case that only affects very specific usage (priority) labels Aug 1, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 1, 2023
@natemoo-re
Copy link
Member

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.

@natemoo-re natemoo-re added the help wanted Please help with this issue! label Aug 1, 2023
@jocchong
Copy link

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
Copy link
Member

@jocchong you might want to hold off on a PR until #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
Copy link

@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

ph1p added a commit to ph1p/astro that referenced this issue Aug 25, 2024
ph1p added a commit to ph1p/astro that referenced this issue Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) help wanted Please help with this issue! pkg: preact Related to Preact (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants