preactjs / signals

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

Removed difference in behaviour of @preact/signals and @preact/signal… #387

Closed XantreDev closed 1 year ago

XantreDev commented 1 year ago

Resolves #https://github.com/preactjs/signals/issues/383

When we return a VNode in a signal/computed we would incorrectly try to treat this as a text-node, which would result in us incorrectly triggering changes

changeset-bot[bot] commented 1 year ago

πŸ¦‹ Changeset detected

Latest commit: 7d5ad6e573f30abb668dd40031592026f2de4baf

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 ready!

Name Link
Latest commit 7d5ad6e573f30abb668dd40031592026f2de4baf
Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/64c235500354dc0008344203
Deploy Preview https://deploy-preview-387--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 1 year ago

@andrewiggins i think it has no performance overhead in case of rendering string into Text nodes directly, but adds oprtunity to render jsx inside signal. Researching preact source code was fun.

There are oprtunity when chaning signal type jsx -> string, to directly create Text node and replace it in this.base and this.__v.__e, but i am not sure will it give real performance boost

XantreDev commented 1 year ago

I will add changeset after review

XantreDev commented 1 year ago

@JoviDeCroock please check this PR πŸ™πŸ™πŸ™

XantreDev commented 1 year ago

Fixed comments @JoviDeCroock

JoviDeCroock commented 1 year ago

This also needs a changeset and probably a rebase/merge so the netlify checks start succeeding

XantreDev commented 1 year ago

@JoviDeCroock please check)