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

A mySignal.value in an unused execution path will not trigger updates #621

Open
2 of 5 tasks
ackvf opened this issue Nov 21, 2024 · 1 comment
Open
2 of 5 tasks
Labels
documentation Improvements or additions to documentation

Comments

@ackvf
Copy link

ackvf commented Nov 21, 2024

  • Check if updating to the latest version resolves the issue
    current: 1.3.0

Environment

  • I am using @preact/signals-core
  • I am using @preact/signals
  • I am using @preact/signals-react

Describe the bug

Hi, this might not be a bug as it might be obvious to some, but for me, a Preact/Signals user of two weeks, this was a bit of a gotcha as I don't think it was mentioned in the docs and a colleague told me that accessing the .value is what causes the magic. Although in hindsight, it does make sense if I think about it. To my defense, though, I thought that it is done (the detecting of functions that access signals.value) via static code analysis, whereas it seems that it is done by actual runtime execution path. - again, it really makes sense.

To Reproduce
Please provide a link to a StackBlitz/CodeSandbox/Codepen project or a GitHub repository that demonstrates the issue.

Steps to reproduce the behavior:

// This will not work
  useSignalEffect(() => {
    const { id } = $currentAction.peek()
    if (!id) return
    const m = $allStates.value[id]
    console.log(m)
  }

// This will work
  useSignalEffect(() => {
    const m_ = $allStates.value
    const { id } = $currentAction.peek()
    if (!id) return
    const m = m_[id]
    console.log(m)
  }

Expected behavior
What should have happened when following the steps above?

There are two things that could be made better

  • Improve documentation to explicitly mention that a mySignal.value needs to be reached and accessed ("accessing" mySignal.value is not enough if it is not reached and executed, or in other words "accessed" - a term used in the documentation, but which kind of obscures the actual message).
  • Create an eslint rule that would warn of this use case - a conditional signal or an early return in front of a signal

And maybe even write a recommendation to adopt this style to define signals upfront.

function fun() {
+  const value = mySignal.value
  // ... much later
 if (someCondition) {
  // do something with mySignal here
+  console.log(value)
-  console.log(mySignal.value)
  }
@rschristian
Copy link
Member

rschristian commented Nov 22, 2024

Indeed, you have it right -- signal.value must be read at runtime to trigger the subscription. Could do with some better docs here & on our site regarding this.

I don't think an eslint plugin is in scope, though if you wanted to build & maintain such a thing, by all means.

@rschristian rschristian added the documentation Improvements or additions to documentation label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants