denoland / fresh

The next-gen web framework.
https://fresh.deno.dev
MIT License
12.56k stars 648 forks source link

Docs about sharing state between islands are misleading #2264

Closed adamzerner closed 9 months ago

adamzerner commented 10 months ago

Here is the page I am referring to: https://fresh.deno.dev/docs/examples/sharing-state-between-islands.

I believe it implies that you need to use signals to share state between islands. However, that is untrue. useState will work as well.

import { Button } from "../button.tsx";
import { useState } from "preact/hooks";

interface CounterProps {
  start: number;
}

// This island is used to display a counter and increment/decrement it. The
// state for the counter is stored locally in this island.
export const Counter = (props: CounterProps) => {
  const [count, setCount] = useState(props.start);

  return (
    <div class="flex gap-2 items-center w-full">
      <p class="flex-grow-1 font-bold text-xl">{count}</p>
      <Button onClick={() => setCount((c) => c - 1)} icon={<span>stuff</span>}>
        -1
      </Button>
      <Button onClick={() => setCount((c) => c + 1)}>+1</Button>
    </div>
  );
};
CAYdenberg commented 9 months ago

The key point about signals is sharing state between islands. If you have two different islands (or even two instances of the same island), they don't have any way to talk to each other without signals.

To illustrate this for yourself, change routes/index.tsx to use two instances of the Counter component in your example.

adamzerner commented 9 months ago

The following indicates that you can in fact have sibling islands with shared state without using signals.

routes/test.tsx

import { Sliders } from "@/islands/sliders.tsx";

export default () => {
  return (
    <main>
      <h1>Demo</h1>
      <Sliders />
    </main>
  );
};

islands/sliders.tsx

import { SynchronizedSlider } from "@/islands/synchronized-slider.tsx";
import { useState } from "preact/hooks";

export const Sliders = () => {
  const [value, setValue] = useState(50);
  const handleChange = (e: any) => {
    setValue(e.target.value);
  };

  return (
    <div>
      <SynchronizedSlider value={value} onInput={handleChange} />
      <SynchronizedSlider value={value} onInput={handleChange} />
      <SynchronizedSlider value={value} onInput={handleChange} />
    </div>
  );
};

islands/synchronized-slider.tsx

import { JSX } from "preact";

export const SynchronizedSlider = (
  { value, onInput }: {
    value: number;
    onInput: JSX.GenericEventHandler<HTMLInputElement>;
  },
) => {
  return (
    <input
      class="w-full"
      type="range"
      min={1}
      max={100}
      value={value}
      onInput={onInput}
    />
  );
};
marvinhagemeister commented 9 months ago

@adamzerner The SynchronizedSlider component is used inside another island directly. Whenever that happens it's not treated as an island, but just like any other component. Only the topmost island component matters and is treated as the boundary.

function Island() {
  return <h1>hello</h1>
}

function OuterIsland() {
  // here component Island is not an island anymore,
  // but treated like any other component
  return <Island />
}

// routes/index.tsx
export default function Page() {
  // here Island is kept as an island, because it's used
  // outside of an island
  return <Island />
}
adamzerner commented 9 months ago

@marvinhagemeister Ahhh, I am seeing now. So you're saying that in my example, it is not sharing state between islands, just within a single island, in which case of course useState will work. Thank you for clarifying and helping me to understand.

Still, I think my original proposal stands: I don't think the docs make this very clear, and so IMO it would be worth updating them. Up to the Fresh team of course though.