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

Broken build with typescript@2.9.0-rc #174

Closed
gcanti opened this issue May 17, 2018 · 15 comments
Closed

Broken build with typescript@2.9.0-rc #174

gcanti opened this issue May 17, 2018 · 15 comments

Comments

@gcanti
Copy link
Owner

gcanti commented May 17, 2018

Here's the error

export type mixed = object | number | string | boolean | symbol | undefined | null
export type Is<A> = (m: mixed) => m is A // <= new error
/*
A type predicate's type must be assignable to its parameter's type.
  Type 'A' is not assignable to type 'mixed'.
    Type 'A' is not assignable to type 'object'.
*/    

Related: section "Unconstrained type parameters are no longer assignable to object in strictNullChecks" in https://blogs.msdn.microsoft.com/typescript/2018/05/16/announcing-typescript-2-9-rc/

What's the better fix? Changing mixed to

export type mixed = {} | undefined | null

?

EDIT: best bet so far is

export type mixed = { [key: string]: any } | object | number | string | boolean | symbol | undefined | null
@dippi
Copy link

dippi commented May 30, 2018

Why don't you add an upper-bound constraint? (As also mentioned in the linked blog post)

export type Is<A extends mixed> = (m: mixed) => m is A

The quest for the perfect definition of mixed still remains, but with a lower priority.

@gcanti
Copy link
Owner Author

gcanti commented May 30, 2018

@dippi because that would mean to add an upper-bound constraint everywhere. It would be awkward and, in practice, a breaking change.

I'd rather prefer to find a good definition for mixed. The above candidate seems to work fine based on my experiments but I need some help to test it out in real code bases, especially those containing custom types / combinators.

@dippi
Copy link

dippi commented May 30, 2018

I see, I'm aware of that issue and I agree with your concerns.
Still I don't know which is the lesser evil, because that definition of mixed leaves a dangerous loophole:

function foo(m: mixed) {
    if (m !== null && typeof m === "object" && "bar" in m) {
        const bar = m["bar"] // bar has type any
        bar.baz // and the compiler won't complain here
    }
}

Following my gut I like better the following definition, that needs the upper-bound constraint, but it's less awkward / dangerous to check and access.

export type mixed = { [key: string]: mixed } | number | string | boolean | symbol | undefined | null;

(note: I'm aware of recursive types limitations and the use of interfaces as a workaround, however I just tried it and, to my surprise, iff the type is exported it seems to work fine)

However I honestly cannot fully picture how much awkward it would become and I must admit I don't even use this library, I was just searching for a good definition of mixed and inspiration for run-time type checking when I stumbled upon this issue. So I'll leave the answer to the owners of real code bases.

@sledorze
Copy link
Collaborator

Unknown type is currently under consideration in typescript

@gcanti
Copy link
Owner Author

gcanti commented May 30, 2018

I must admit I don't even use this library

Well, then thank you for taking the time to chime in, more opinions/suggestions are always helpful.

that definition of mixed leaves a dangerous loophole

You are right, my candidate is not a perfect fit for a general purpose definition of mixed.
However looks good enough for io-ts, let me explain what I mean. I chose { [key: string]: any } because it plays well with t.Dictionary. In fact, in "idiomatic" io-ts you would define foo as

function foo(m: t.mixed) {
  if (t.Dictionary.is(m) && 'bar' in m) {
    const bar = m['bar'] // bar has type mixed
    bar.baz // and the compiler DOES complain here
  }
}

Finally, as @sledorze pointed out, in the TypeScript repo there's some work related to unknown (microsoft/TypeScript#24439) and any (microsoft/TypeScript#24423) so maybe we should just wait the next release and then try to change mixed to

type mixed = unknown

@gcanti
Copy link
Owner Author

gcanti commented May 31, 2018

maybe we should just wait...

FYI just tried typescript@3.0.0-dev.20180531 (which already contains unknown) and type mixed = unknown works like a charm

@sledorze
Copy link
Collaborator

@gcanti what do you think about the proper release / schedule (make it available in 'next' ?)

@gcanti
Copy link
Owner Author

gcanti commented May 31, 2018

@sledorze my plan would be

  • wait for the next stable release (I guess typescript@2.9.1) which should contain unknown
  • change mixed into an unknown alias (*)
  • gather feedback: is everthing ok in your code bases?
  • release the change as io-ts@1.2.0

(*) you can try it now editing node_modules/io-ts/lib/index.d.ts

@gcanti gcanti added this to the 1.2.0 milestone May 31, 2018
@sledorze
Copy link
Collaborator

No errors at compilation time.
Looks good to me 👍

@gcanti
Copy link
Owner Author

gcanti commented May 31, 2018

wait for the next stable release (I guess typescript@2.9.1)

My guess was wrong, unknown is coming in TypeScript 3, see Announcing TypeScript 2.9.1

@sledorze so in the meanwhile I would go for

export type mixed = { [key: string]: any } | object | number | string | boolean | symbol | undefined | null

WDYT?

artfwo added a commit to freight-hub/TypedEnv that referenced this issue Jun 1, 2018
@sledorze
Copy link
Collaborator

sledorze commented Jun 1, 2018

@gcanti I need to test it!

@sledorze
Copy link
Collaborator

sledorze commented Jun 1, 2018

@gcanti not compilation error on the code base using your latest proposal.

@spacejack
Copy link

My project uses io-ts. With that updated definition for mixed I can compile and run it fine with typescript 2.9.1.

@mlegenhausen
Copy link
Contributor

@gcanti works also for me withtypescript@2.9.1.

@gcanti
Copy link
Owner Author

gcanti commented Jun 6, 2018

Thanks @sledorze @spacejack @mlegenhausen, I'm going to release a patch

@gcanti gcanti closed this as completed in 670c8e4 Jun 6, 2018
@gcanti gcanti removed this from the 1.2.0 milestone Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants