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

[experiment] Improve Effects assignability after adding Symbol.iterator by replacing voids with unknown #2636

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link

This is not for merging, I just put it together since I was curious if I could make it work this way. This work was started based on the initial commit from #2602

It doesn't (yet?) allow you to remove the adapter thing from this branch but perhaps this is just missing some minor thing. I'm not sure if there were any extra changes done by @tim-smart to allow that. This PR focuses solely on making this to typecheck with the added Symbol.iterator method.

There are a few minor TODO comments here but all of them (but one!) are really about the same thing - adding Symbol.iterator properly to your error classes.

Copy link

changeset-bot bot commented Apr 27, 2024

⚠️ No Changeset found

Latest commit: 7da71f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mikearnaldi mikearnaldi marked this pull request as draft April 27, 2024 08:59
@mikearnaldi
Copy link
Member

Nice!

@mikearnaldi
Copy link
Member

This is part of a larger discussion about using "void" instead of "unknown" when we don't care about the value of an effect, in theory it would make sense

@tim-smart
Copy link
Member

Thanks for taking the time to look at this :)

Replacing void with unknown does feel weird, feels like we are losing some semantics by making the return types too generic.

@Andarist
Copy link
Author

What kind of semantics you'd lose? I find void to be quite weird and problematic at times. Take a look at this:

declare const test: string | void | undefined;

if (test === undefined) {
  test; // void | undefined
} else {
  test; // string
}

@tim-smart
Copy link
Member

Mainly you lose some meaning / intent. This function returns nothing (void) vs this function could return anything but we don't care what (unknown).

@mikearnaldi
Copy link
Member

Mainly you lose some meaning / intent. This function returns nothing (void) vs this function could return anything but we don't care what (unknown).

Void doesn't mean that this function returns nothing, it means that we don't care about the return

@Andarist
Copy link
Author

Yeah, so in that sense usually either unknown or undefined is (IMHO) a better more-predictable choice (depending on your needs).

This is perfectly valid:

const test: () => void = () => 42;

@tim-smart
Copy link
Member

tim-smart commented Apr 29, 2024

But in terms of familiarity for the majority of developers, most of them know that

const myFn = () => {
  // No explicit return
}

Will infer a return type of void. I just think that familiarity is worth something.

@mikearnaldi
Copy link
Member

But in terms of familiarity for the majority of developers, most of them know that

const myFn = () => {
  // No explicit return
}

Will infer a return type of void. I just think that familiarity is worth something.

yes it will infer void, but you can also type void and return, the meaning really is "whatever" and given it is sometimes treated as undefined it creates potential issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Discussion Ongoing
Development

Successfully merging this pull request may close these issues.

None yet

3 participants