preactjs / signals

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

signals-react doesn't work in react production build #155

Closed dackom closed 2 years ago

dackom commented 2 years ago

The basic example is not working on production build in react:

const count = useSignal(1);
return (
<div>
   {count}
   <button onClick={() => count.value++}>INC</button>
</div>
);

Versions:

    "@preact/signals-react": "^1.0.2",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",

Building with vite 3.1.0

developit commented 2 years ago

@marvinhagemeister I wonder if we missed the publish for the react integration too, like in #140?

marvinhagemeister commented 2 years ago

@developit Can confirm that it's not a bad publish, but a bug. It seems like the signal isn't subscribed to.

Looks like our lastOwner variable is never set in production. Yep that's what's happening. We never create an updater function because of that.

An-Huynh-Certivity commented 2 years ago

EDIT: Turns out I am stupid. You need to put the signal instantiation outside of the component or it will run on every rerender. This also seems like it could be the solution for the original issue.

I am having this exact same issue running in vite + react (both locally in a freshly created vite project and the stackblitz template provided on https://vitejs.dev/guide/)

import { effect, signal } from '@preact/signals-react'

function Test() {
  const foo = signal<boolean>(false)
  effect(() => {
    console.log(foo.value)
  })

  const counter = signal(0)
  effect(() => {
    console.log(counter.value)
  })

  return (
    <>
      <div>
        <div>{foo.value ? 'true' : 'false'}</div>
        <button
          className="bg-gray-400"
          onClick={() => {
            foo.value = !foo.value
          }}
        >
          invert
        </button>
      </div>
      <div>
        <div>{counter.value}</div>
        <button
          className="bg-gray-400"
          onClick={() => {
            counter.value += 1
          }}
        >
          increment
        </button>
      </div>
    </>
  )
}

export default Test

with the same package.json as the original post.

On invert, the console logs:

true
false
0

On increment, the console logs:

1
false
0
mohammadraufzahed commented 2 years ago

any progress?

jmeistrich commented 2 years ago

For what it's worth I haven't found ReactCurrentOwner to be dependable. In my testing in production it seems to always be set to the root node, and in React Native it's even less dependable. So for Legend-State's implementation I used only ReactCurrentDispatcher to track lifecycle, which does work in production and React Native.

I'm interested to see if you're able to find a workable solution with ReactCurrentOwner (or something else) but it may not be possible.

developit commented 2 years ago

@jmeistrich are you ending up with two copies of React?

jmeistrich commented 2 years ago

@developit No, I don't think so. I've been testing in three different production apps (React 17, 18, React Native) and I don't see consistent ReactCurrentOwner behavior. In production sometimes it's set to the root node, sometimes it's always null (I don't remember which was which).

But the solution I ended up with was to use only the dispatcher to track lifecycle, mapping to the rerender function rather than the owner, and that is working great. I submitted a PR to use a similar solution for Signals: https://github.com/preactjs/signals/pull/215

marvinhagemeister commented 2 years ago

This is fixed by the fantastic PR from @jmeistrich #215 🎉 We'll cut a new release soon.