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

feat: add isNothing() #989

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

feat: add isNothing() #989

wants to merge 1 commit into from

Conversation

unional
Copy link
Contributor

@unional unional commented Oct 30, 2022

fix #988

I can add tests, but want to know which file should I add it to. Type guard is TS stuff so maybe I create a new file __tests__/common.ts?

@netlify
Copy link

netlify bot commented Oct 30, 2022

Deploy Preview for quizzical-lovelace-dcbd6a canceled.

Name Link
🔨 Latest commit 54f63e1
🔍 Latest deploy log https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/635dd3788f41e6000957947c

@unional
Copy link
Contributor Author

unional commented Oct 30, 2022

Another thing is, should the Nothing type be exposed?

Right now I have to access it through typeof nothing

Copy link
Collaborator

@mweststrate mweststrate left a comment

Choose a reason for hiding this comment

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

Sorry for the late response! Feature looks good time, as does the test proposal. Would you mind to add the test file indeed, testing both the type inference and runtime behavior of the utility? Thanks!

@unional
Copy link
Contributor Author

unional commented Jan 2, 2023

Would you mind to add the test file indeed, testing both the type inference and runtime behavior of the utility?

Sure, do you mind I add type-plus to the dev dependency so that I can easily test the types?

@unional
Copy link
Contributor Author

unional commented Jan 3, 2023

Also, looking at the PR again, it turns out the type is actually not exported. Will need to fix that.

The circular dependency is also an issue. Maybe I can create a different PR to fix all circular dependency issue first, as that would lead to many problems.

@mweststrate
Copy link
Collaborator

Sure, do you mind I add type-plus to the dev dependency so that I can easily test the types?

We already use spec.ts (see e.g. produce.ts), or using // @ts-expected-error might suffice?

@unional
Copy link
Contributor Author

unional commented Jan 4, 2023

I'll check that out. Thx

@mweststrate
Copy link
Collaborator

@unional were you able to set up some tests?

@unional
Copy link
Contributor Author

unional commented Jan 15, 2023

Hi, yes I just get back to this.

I have tried the spec.ts and // @ts-expected-error and saw how you use it:

// @ts-expect-error
assert(value, _ as Nothing)

// vs
isType.equal<false, typeof value, Nothing>()

It works either way. Didn't know about @ts-expect-error. Thanks!.

I do found one issue from this PR.
The function isNothing() depends on the Nothing type, but immer implement this mechanism using the a dummy class Nothing.

I looked up the code and found that it is actually needed by one of the public types already:

/** Converts `nothing` into `undefined` */
type FromNothing<T> = T extends Nothing ? undefined : T

/** The inferred return type of `produce` */
export type Produced<Base, Return> = Return extends void
	? Base
	: Return extends Promise<infer Result>
	? Promise<Result extends void ? Base : FromNothing<Result>>
	: FromNothing<Return>

Produced -> FromNothing -> Nothing.

So exposing the type should be needed in the first place and that may be an existing bug.
But testing and proving that could be difficult.

However, exposing this Nothing class is not good because it is not used as a class but a type.
One way to fix this is replace the mechanism with a branded type, e.g. https://github.com/unional/type-plus/blob/main/ts/nominal-types/Brand.ts#L11

But that will need some investigation.

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.

add IsNothing() type guard
2 participants