preactjs / signals

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

Changes void to unknown for callback definitions. #462

Open MicahZoltu opened 7 months ago

MicahZoltu commented 7 months ago

A callback like () => void means that the caller must provide a function that is explicitly void returning. This means one cannot pass a function as a callback that may have a return value that is ignored. By changing the required return type to unknown, it allows the caller to provide either a void returning function, or a function that returns a value.

This is especially valuable when you have callback functions like () => condition && doThing(). Such a function returns a boolean value, which makes it incompatible with () => void callback requirements. You can work around this by wrapping the body with curly braces (to throw away the result of the expression), but it is better to be permissive in what is accepted instead.

changeset-bot[bot] commented 7 months ago

🦋 Changeset detected

Latest commit: 378c56c1874db8fe3ddf57625fdfd8ccf9c85f33

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

Deploy Preview for preact-signals-demo ready!

Name Link
Latest commit 378c56c1874db8fe3ddf57625fdfd8ccf9c85f33
Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/6581439bc5c4e60008d0b541
Deploy Preview https://deploy-preview-462--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.

MicahZoltu commented 7 months ago

I ran pnpm changeset, but no idea if I did so correctly.

Regarding react, I don't use/haven't used react so I'm not sure how this would be ported over to that.

rschristian commented 7 months ago

I ran pnpm changeset, but no idea if I did so correctly.

Looks great, thanks! Just helps us keep track of changes and what needs to be released.

Regarding react, I don't use/haven't used react so I'm not sure how this would be ported over to that.

I might be able to take a look later, but generally it would just be applying the same thing I'd think (+ to effect())? I haven't taken a look at our React adapter in a while though, if it's way more complex then no worries, we can just merge this in.

MicahZoltu commented 7 months ago

I do see this, which certainly seems related. Does that need to be changed as well? https://github.com/preactjs/signals/blob/main/packages/react/runtime/src/index.ts#L378