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

Changes void to unknown for callback definitions. #462

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MicahZoltu
Copy link

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.

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.
Copy link

changeset-bot bot commented Dec 13, 2023

🦋 Changeset detected

Latest commit: 378c56c

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

Copy link

netlify bot commented Dec 13, 2023

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit 378c56c
🔍 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.

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me -- can this be mirrored into core & react (where applicable)? I imagine our types there will also need an adjustment.

Also needs a changeset, run pnpm changeset from the project root please.

@MicahZoltu
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@@ -356,7 +356,7 @@ export function useComputed<T>(compute: () => T) {
return useMemo(() => computed<T>(() => $compute.current()), []);
}

export function useSignalEffect(cb: () => void | (() => void)) {
export function useSignalEffect(cb: () => unknown | (() => unknown)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the type of the callback passed to effect? Should this type match that since this callback is invoked inside of an effect and its return value is passed to effect? IIRC, effect accepts a clean up function

Copy link
Author

@MicahZoltu MicahZoltu Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point. We could make the change from

useEffect(() => {
	return effect(() => callback.current());
}, []);

to

useEffect(() => {
	return effect(() => { callback.current() });
}, []);

This would ensure that effect is getting a void returning function. Alternatively, we could change the definition of effect, but I think that is in a different repository?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and made the above change to this PR, can revert if another solution is desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants