preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.71k stars 91 forks source link

feat(signals-preact): useLiveSignal #367

Closed JoviDeCroock closed 1 year ago

JoviDeCroock commented 1 year ago

Resolve #366

This adds useLiveSignal as a way to wrap a signal passed into a component which can change reference.

changeset-bot[bot] commented 1 year ago

đŸĻ‹ Changeset detected

Latest commit: 525c50e67d79aae9538fb5dd4a1bdbcd389665e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | --------------- | ----- | | @preact/signals | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 1 year ago

Deploy Preview for preact-signals-demo failed.

Name Link
Latest commit 525c50e67d79aae9538fb5dd4a1bdbcd389665e7
Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/646efe7b7ae2050008eade3d
github-actions[bot] commented 1 year ago

Size Change: +298 B (0%)

Total Size: 69.2 kB

Filename Size Change
docs/dist/assets/index.********.js 1.08 kB +243 B (+29%) 🚨
packages/preact/dist/signals.js 1.23 kB +27 B (+2%)
packages/preact/dist/signals.mjs 1.18 kB +28 B (+2%)
ℹī¸ View Unchanged | Filename | Size | | :--- | :---: | | `docs/dist/assets/client.********.js` | 46.5 kB | | `docs/dist/assets/jsxRuntime.module.********.js` | 282 B | | `docs/dist/assets/preact.module.********.js` | 4 kB | | `docs/dist/assets/signals-core.module.********.js` | 1.42 kB | | `docs/dist/assets/signals.module.********.js` | 1.97 kB | | `docs/dist/assets/style.********.js` | 21 B | | `docs/dist/assets/style.********.css` | 1.21 kB | | `docs/dist/basic-********.js` | 244 B | | `docs/dist/demos-********.js` | 3.35 kB | | `docs/dist/nesting-********.js` | 1.13 kB | | `docs/dist/react-********.js` | 237 B | | `packages/core/dist/signals-core.js` | 1.48 kB | | `packages/core/dist/signals-core.mjs` | 1.5 kB | | `packages/react/dist/signals.js` | 1.21 kB | | `packages/react/dist/signals.mjs` | 1.15 kB |

compressed-size-action

andrewiggins commented 1 year ago

Oohh interesting. I hadn't thought of this kind of use case before. Thanks for bringing this up!

One thought about this new hook is that it requires the child component (Todo in the Readme example) to handle if its parent might pass in a new signal object as a prop. It seems to me all components might want to call useLiveSignal instead of useSignal to handle getting a new signal object since it can't know how it might be called. Could the number of .values you have to deference depend on how many calls to useLiveSignal your parents have invoked on that signal?

I wonder if there are other patterns we could instead encourage to handle the scenario mentioned in the Readme. It appears there are two signals that hold a value for which there is another "current" value. Could this "current" value be a signal, i.e. currentDate? The current date depends on the on React state so wherever it is updated the currentDate signal would also need to be updated. So the button would become something like <button onClick={() => { setDisplayed(!on); currentDate.value = !on ? dateA : dateB;}>.

Though having to change state twice (once for React state and another for signal state) isn't great. It'd be nice to just do useComputed(() => on ? dateA : dateB); or something but since on is React state and not a signal, this computed value won't rerun when on changes. This problem makes me think another issue we have is mixing & synchronizing React state and Signal state. I wonder if we should add a hook to help that situation to encourage people to not change signal instances and instead create computed signals to represent the final value. Perhaps something like const [ state, setState, stateSignal] = useSignalState() which looks and behaves like React.useState but also exposes the state as a signal that can be used in things like useComputed (though synchronizing the UI when some UI updates synchronously (UI powered by stateSignal) and others update asynchronously (UI powered by setState) (maybe using useSyncExternalStore could help us here?

I'd be curious if there are other situations where switching signal instances in rendering may be relevant cuz perhaps I need to dwell on it a bit more to better understand it use cases.

Anyways, that's a lot of thoughts. Would love to hear what you think!

XantreDev commented 1 year ago

Why we cannot do it like that?

import { useComputed, useLiveSignal, useSignal } from '@preact/signals';

// The signal representing props.date can change
// when i.e. someone presses a butotn
function Todo({ date }) {
  const formattedDate = useComputed(() => formatDate(liveDateSignal.value));
  return <span>{formattedDate}</span>;
}

function App() {
  const dateA = useSignal(0);
  const dateB = useSignal(1);
  const on = useSignal(true)

  return (
    <main>
      <button onClick={() => {on.value = !on.peek()}}>
        Toggle
      </button>
        <Todo date={useComputed(() => on.value ? dateA.value : dateB.value)} />
    </main>
  );
}
XantreDev commented 1 year ago

I think in most cases when we are writing component powered by signals we should stick with then and think about component like about solidjs-like constructor function

JoviDeCroock commented 1 year ago

@andrewiggins I see your point, I honestly dislike the .value.value consequence of having to access it this way. The main problem here becomes that the reference to the signal isn't safe so maybe just incorporating this in useSignal is the most intuitive way of incorporating this however that probably would need to be stateful i.e. rather than useSignal using useMemo to just create a signal once it would need to create it in state and check whether it's referentially equal on each invocation. WDYT?

Or we could just opt to drop this entirely and bet more on education for signals, as I guess this fundamentally could be considered education something along the lines of, if the reference to a signal can change you have to use a key as that would destroy the component and maintain the correct subscriptions.

XantreDev commented 1 year ago

@JoviDeCroock maybe it can be solved with eslint plugin for signals that will enforce stable reference to signals