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

feat(signals-preact): useLiveSignal #367

Closed
wants to merge 2 commits into from
Closed

feat(signals-preact): useLiveSignal #367

wants to merge 2 commits into from

Conversation

JoviDeCroock
Copy link
Member

Resolve #366

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

@changeset-bot
Copy link

changeset-bot bot commented May 25, 2023

🦋 Changeset detected

Latest commit: 525c50e

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

netlify bot commented May 25, 2023

Deploy Preview for preact-signals-demo failed.

Name Link
🔨 Latest commit 525c50e
🔍 Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/646efe7b7ae2050008eade3d

@github-actions
Copy link
Contributor

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

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

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

XantreDev commented Jul 5, 2023

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

JoviDeCroock commented Jul 7, 2023

@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
Copy link
Contributor

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

@JoviDeCroock JoviDeCroock deleted the live-signal branch September 6, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug?: useComputed doesn't update when given a new signal
3 participants