Skip to content

feat: add isNothing() - #989

Open
unional wants to merge 1 commit into
immerjs:mainfrom
unional:isNothing
Open

feat: add isNothing()#989
unional wants to merge 1 commit into
immerjs:mainfrom
unional:isNothing

Conversation

@unional

Copy link
Copy Markdown
Contributor

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

netlifyBot commented Oct 30, 2022

Copy link
Copy Markdown

Deploy Preview for quizzical-lovelace-dcbd6a canceled.

NameLink
🔨 Latest commit54f63e1
🔍 Latest deploy loghttps://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/635dd3788f41e6000957947c

@unional

Copy link
Copy Markdown
ContributorAuthor

Another thing is, should the Nothing type be exposed?

Right now I have to access it through typeof nothing

@mweststratemweststrate left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

I'll check that out. Thx

@mweststrate

Copy link
Copy Markdown
Collaborator

@unional were you able to set up some tests?

@unional

unional commented Jan 15, 2023

Copy link
Copy Markdown
ContributorAuthor

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-errorassert(value,_asNothing)// vsisType.equal<false,typeofvalue,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` */typeFromNothing<T>=TextendsNothing ? undefined : T/** The inferred return type of `produce` */exporttypeProduced<Base,Return>=Returnextendsvoid
? Base
: ReturnextendsPromise<infer Result>
? Promise<Resultextendsvoid ? 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.

@unional

Copy link
Copy Markdown
ContributorAuthor

Updated branch and code. Added directly to immer.ts as it is exporting NOTHING as nothing there.

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 20424962423

Details

  • 1 of 3(33.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 42.86%

Changes Missing CoverageCovered LinesChanged/Added Lines%
src/immer.ts1333.33%
TotalsCoverage Status
Change from base Build 20394930238:-0.02%
Covered Lines:1679
Relevant Lines:4774

💛 - Coveralls

@unional

Copy link
Copy Markdown
ContributorAuthor

Want to share that since v10 of using symbol, it has a drawback of not able to distribute the union type with Promise.
i.e.:

typeUpdater=(s: any)=>number|typeofNOTHING|Promise<number|typeofNOTHING>// error hereconstu: Updater=async()=>NOTHING
Type 'Promise<symbol>' is not assignable to type 'number | unique symbol | Promise<number | unique symbol>'.
Type 'Promise<symbol>' is not assignable to type 'Promise<number | unique symbol>'.
Type 'symbol' is not assignable to type 'number | unique symbol'.ts(2322)

An explicit type declaration is needed:

constu: Updater=async(): Promise<typeofNOTHING>=>NOTHING

I believe this is a limitation on TypeScript.

Sign up for freeto 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

3 participants

@unional@mweststrate@coveralls