Skip to content

is type guards for properties #11796

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

Closed
thorn0 opened this issue Oct 22, 2016 · 12 comments
Closed

is type guards for properties #11796

thorn0 opened this issue Oct 22, 2016 · 12 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@thorn0
Copy link

thorn0 commented Oct 22, 2016

TypeScript Version: 2.0.3

Code

From the typings for the vinyl module.

declare class File {
    // ...
    /**
     * Type: Buffer|Stream|null (Default: null)
     */
    public contents: Buffer | NodeJS.ReadableStream;

    /**
     * Returns true if file.contents is a Buffer.
     */
    public isBuffer(): boolean;

    /**
     * Returns true if file.contents is a Stream.
     */
    public isStream(): boolean;

    /**
     * Returns true if file.contents is null.
     */
    public isNull(): boolean;

    // ...
}

Expected behavior:

Possibility to specify something like this.contents is Buffer instead of just boolean for the return type of the methods isBuffer, isStream, isNull.

Actual behavior:

The language doesn't support this.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Oct 22, 2016

The language doesn't support this.

This is simple and easy to do in TypeScript, no pun intended
EDIT: pun intended 😜

declare class File {
  // ...
  /**
   * Type: Buffer|Stream|null (Default: null)
   */
  private contents: Buffer | NodeJS.ReadableStream;

  /**
   * Returns true if file.contents is a Buffer.
   */
  public isBuffer(): this is { contents: Buffer };

  /**
   * Returns true if file.contents is a Stream.
   */
  public isStream(): this is { contents: NodeJS.ReadableStream };
}

const f = new File();

if (f.isBuffer()) {
  console.log(f.contents.buffer);
} else if (f.isStream()) {
  console.log(f.contents.read(1024));
} 

@aluanhaddad
Copy link
Contributor

Note, you can also write

isBuffer(): this is this & { contents: Buffer };

Which will propagate the instance type for further refinement while allowing you to access contents as Buffer.

@thorn0
Copy link
Author

thorn0 commented Oct 22, 2016

However, it doesn't work exactly like type guards usually work. Inside this if, I'd expect f.contents to be never, but it's Buffer & NodeJS.ReadableStream:

if (f.isBuffer() && f.isStream()) { ... }

Still great that it works.

@aluanhaddad
Copy link
Contributor

@thorn0, in the future subset types might be able to help with this. For now, you can use this, which is actually shorter and arguably more readable

interface FileBuffer {
  kind: 'buffer';
  contents: Buffer;
}
interface FileStream {
  kind: 'stream';
  contents: NodeJS.ReadableStream;
}

type File = FileBuffer | FileStream;

declare const File: new () => File;

const f = new File();
if (f.kind === 'stream' && f.kind === 'buffer') // error

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Oct 24, 2016
@zpdDG4gta8XKpMCd
Copy link

true and false a type literals now, take advantage of it

interface FileBuffer {
  isBuffer: true;
  contents: Buffer;
}
interface FileStream {
  isBuffer: false;
  contents: NodeJS.ReadableStream;
}

type File = FileBuffer | FileStream;
declare const File: new () => File;

const f = new File();
if (f.isBuffer) {
    f.contents; // Buffer
} else {
    f.contents; // NodeJS.ReadableStream
}

@thorn0
Copy link
Author

thorn0 commented Oct 24, 2016

I'm not the author of the library. I just wanted to improve its type definitions.

@aluanhaddad
Copy link
Contributor

@Aleksey-Bykov Indeed, that is much more elegant.

@thorn0 I messed around a little bit and was able to get the mutually exclusive && case working

interface FileBuffer {
    contents: Buffer;
    isBuffer(): true;
    isStream(): this is never;
}

interface FileStream {
    contents: NodeJS.ReadableStream;
    isStream(): true;
    isBuffer(): this is never;
}

interface File {
    contents: Buffer | NodeJS.ReadableStream;
    isBuffer(): this is FileBuffer;
    isStream(): this is FileStream;
}

declare const File: new () => File;


// ...


const f = new File();

if (f.isBuffer()) {
    console.log(f.contents.buffer);
}
if (f.isStream()) {
    console.log(f.contents.read(1024));
}

if (f.isBuffer() && f.isStream()) {
    f.contents; //'Property 'contents' does not exist on type 'never'.
}

Not exactly what you asked for in that f is never, rather than f.contents, but I think it matches the usecase. This definition should be compliant with the behavior of the library itself.

@thorn0
Copy link
Author

thorn0 commented Oct 31, 2016

@aluanhaddad It works. :) Ta-da: DefinitelyTyped/DefinitelyTyped#12368 🎉

@RyanCavanaugh
Copy link
Member

@thorn0 should I leave this issue open?

@thorn0
Copy link
Author

thorn0 commented Oct 31, 2016

I honestly don't know. The solution that @aluanhaddad found looks hackish, but it allowed me to achieve what I wanted. So far it's the first and only time I needed a type guard like this.prop is Type. Probably the language doesn't really need such a feature. What do you think?

@RyanCavanaugh
Copy link
Member

If it can be accomplished through reasonable means I'd prefer to not do extra work / add complexity unless it's really needed. I'll retag for now in case anyone else runs into something similar and can't use the same method.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Oct 31, 2016
@RyanCavanaugh
Copy link
Member

Seems like this is good enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants