Type error in Buffer.from()

[JustinBeckwith]

Greetings folks! 👋
I'm getting a type error I can't explain. I have some code like this:

function doStuff(foo: string|Buffer) {
  return Buffer.from(foo);
}

This gives me the compilation error:

Argument of type 'string | Buffer' is not assignable to parameter of type 'string'. Type 'Buffer' is not assignable to type 'string'.

This is weird, because Buffer.from can accept a string or a Buffer. The d.ts for node.js appears to check out:

    /**
     * Copies the passed {buffer} data onto a new Buffer instance.
     */
    from(buffer: Buffer): Buffer;
    /**
     * Creates a new Buffer containing the given JavaScript string {str}.
     * If provided, the {encoding} parameter identifies the character encoding.
     * If not provided, {encoding} defaults to 'utf8'.
     */
    from(str: string, encoding?: string): Buffer;

This kinda feels like a bug. Am I missing something? Thanks!

cc @ofrobots

[mhegazy]

Overload resolution is not distributed over arguments of union type. See #14107 for more details.

For this specific issue we can take a targeted fix to change the overloads of from to be:

    from(buffer: any[] | ArrayBuffer | string | Buffer): Buffer;
    from(arrayBuffer: ArrayBuffer, byteOffset: number, length?: number): Buffer;
    from(str: string, encoding: string): Buffer;

[mhegazy]

So to be clear, we are accepting a PR for updating the signature of Buffer.from to use union types.
The underlying issue of union arguments and overloads is tracked by #14107.

[a-tarasyuk]

@mhegazy I think this issue is not related to lib.d.ts. Buffer does not exist in Browser API, only in NodeJS. So I suppose fix should be added in DefinitelyTyped/DefinitelyTyped/types/node. What do you think?

[mhegazy]

@mhegazy I think this issue is not related to lib.d.ts. Buffer does not exist in Browser API, only in NodeJS. So I suppose fix should be added in DefinitelyTyped/DefinitelyTyped/types/node. What do you think?

Doh! thanks @a-tarasyuk for the correction. yes this should be done on the DT side. and looks like that you fixed it already in DefinitelyTyped/DefinitelyTyped#24966. thanks!

[inlined]

Sorry to be late to the game; I'm not a fan of the fix:

1. There's a definition for Buffer.from(string) in the Node v4 definition, but the API didn't exist until Node 5.
2. We're missing the second parameter for Buffer.from(string)
3. There is a commented out type name in the parameter list for Buffer.from
4. The methods docs are worse than their predecessor.

Do we really need to muddy the codebase when the workaround is so simple? It seems like the right thing to do is ask someone to "me too" the original issue and add the 3LOC workaround:

function doStuff(foo: string|Buffer) {
  // https://github.com/Microsoft/TypeScript/issues/14107
  if (typeof foo === 'string') {
    // Now my code can even handle strings that were in hex64,
    // not ascii.
    foo = Buffer.from(foo/*, optEncoding */);
  }
  // do real stuff
}

[JustinBeckwith]

It isn't like this is a big deal for us or anything, just found it to be a minor inconvenience and something I should probably report :) I'm perfectly happy to wait for #14107 to land.

[inlined]

Sorry, I didn't mean to bite off anyone's head either. I'm really glad you reported the issue and I hope that the bug leaves a paper trail so more people can add pressure to #14107; the TypeScript team has got loads on their plate and +1s help them balance priorities.

@a-tarasyuk; would it be possible to revert since the fix added bugs? @JustinBeckwith should be able to work around with the if (typeof foo === 'string') clause and it will actually lead to more explicit and self documenting code if he includes the encoding he wants.

[a-tarasyuk]

@inlined sure, I can make PR. The fix is ready just need to get approve from @mhegazy, and make PR on GitHub. Does it work for you?

[inlined]

Yeah. If @mhegazy is onboard

…

On Mon, Apr 16, 2018 at 12:40 AM Alexander T. ***@***.***> wrote: @inlined <https://github.com/inlined> sure, I can make PR. The fix is ready just need to get approve from @mhegazy <https://github.com/mhegazy>, and make PR on GitHub. Does it work for you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23155 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABe1j0ysf3PlWm1EhgBpipGxXKwLh8vEks5tpEsAgaJpZM4THpfV> .

[mhegazy]

I do not think the change is bad really. The order of the overloads can be fixed to address the issues you are referring to earlier. But in general it is better to use union types than having multiple overloads.

[Flarna]

There's a definition for Buffer.from(string) in the Node v4 definition, but the API didn't exist until Node 5.

@inlined It's also available in NodeJS 4 - see https://nodejs.org/dist/latest-v4.x/docs/api/buffer.html#buffer_class_method_buffer_from_str_encoding

[Flarna]

There is a commented out type name in the parameter list for Buffer.from

@inlined I expect you refer to TypedArray. The problem here is that this is a ES6 type and if we change node typings to require lib ES6 we break a lot use code (I did this once....). But Node supports TypedArray here so you can see this a placehoder for the future.

By the way, which exact use is the problem now? I locally added Buffer.from("ffo", "utf-8") in the tests and no complain from typescript. Which way do you call Buffer.from() and which typescript version?

[inlined]

Looks like I've overreacted to everything. I retract my previous comments. Thanks everyone for being patient with me here.

The problem here is that this is a ES6 type and if we change node typings to require lib ES6 we break a lot use code (I did this once....).

Is it not reasonable to expose the type in types/node/v8 and remove the comment elsewhere?

By the way, which exact use is the problem now? I locally added Buffer.from("ffo", "utf-8") in the tests and no complain from typescript.

Yup... A total facepalm moment on my part. I missed the one overload after the GitHub diff collapse.

It's also available in NodeJS 4

Interesting; the v4 docs and 4.5 release notes agree. The latest docs misleadingly say "added in 5.10". Should we not be trusting those docs when reviewing this codebase?

[Flarna]

Is it not reasonable to expose the type in types/node/v8 and remove the comment elsewhere?

I found that my last statement in this is outdated - nodejs typings refer to lib.es6.d.ts already so Int8Array,... are available.
Therefore I think it would be possible to add | Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array or define a type TypedArray = Int8Array | Uint8Array | Uint8ClampedArray | Int16Array | Uint16Array | Int32Array | Uint32Array | Float32Array | Float64Array.

There are a few other locations where TypedArray is commented so they should be adapted also if someone creates a PR for this.

Interesting; the v4 docs and 4.5 release notes agree. The latest docs misleadingly say "added in 5.10". Should we not be trusting those docs when reviewing this codebase?

Well, I did a fast try and then it was clear the v4 doc is correct. I expect this is a side effect of how NodeJs backports new APIs. First they land on master and get released (e.g. in 5.10). The docs on master tell exactly this. A few weeks later the backport is done (for selected features only) by merging the relevant changes to v4 branch - but docs on master are not updated automatically.
I think it's up to us to create a PR towards node doc to fix this 😁