search

preactjs / signals

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

Addes recursive `.value` read detection in nested functions #553

Closed XantreDev closed 2 months ago

XantreDev commented 6 months ago

552

changeset-bot[bot] commented 6 months ago

🦋 Changeset detected

Latest commit: 46a701f7dfe76f42c9a1fd8131eea670f41f1f22

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-react-transform | Patch |

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 6 months ago

Deploy Preview for preact-signals-demo ready!

Name Link
Latest commit 46a701f7dfe76f42c9a1fd8131eea670f41f1f22
Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/665b4ed849db430008146af1
Deploy Preview https://deploy-preview-553--preact-signals-demo.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

XantreDev commented 6 months ago

I've didn't changed useSignals implementation. Don't know why test fails

XantreDev commented 4 months ago

@rschristian I understood why test fails

It transforms unmanaged test to managed test. Which is not working, because of it do not uses queueMicrotask

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/packages/react/runtime/test/browser/useSignals.test.tsx#L495-L527

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/packages/react/runtime/test/browser/useSignals.test.tsx#L456-L458

Should I add workaround to fix transformation behavior?

Should this file even be transformed?

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/karma.conf.js#L157-L169

rschristian commented 4 months ago

I'm not familiar w/ the React transform (or its tests) at all -- I haven't/don't use it.

@andrewiggins is the best person to ask really, I couldn't say.

XantreDev commented 4 months ago

@rschristian I understood why test fails

It transforms unmanaged test to managed test. Which is not working, because of it do not uses queueMicrotask

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/packages/react/runtime/test/browser/useSignals.test.tsx#L495-L527

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/packages/react/runtime/test/browser/useSignals.test.tsx#L456-L458

Should I add workaround to fix transformation behavior?

Should this file even be transformed?

https://github.com/XantreDev/signals/blob/46a701f7dfe76f42c9a1fd8131eea670f41f1f22/karma.conf.js#L157-L169

@andrewiggins What is your thoughts about it?

JoviDeCroock commented 2 months ago

582 should have you covered on this one, will close this one out as superseded by that one