Hacker News Reader

Comment by funkaster | Hacker News Reader

This is wrong: (from the article)

  function f(leaves: Array<Leaf>, aggregators: Array<Aggregator>): Array<MemsqlNode> {
    // The next line errors because you cannot concat aggregators to leaves.
    return leaves.concat(aggregators);
  }

That's just wrong, of course you can't concat aggregators and leaves! and Typescript is OK to not accept it. If you want that to be ok, you could do something like

  [].concat(leaves, aggregators)

View on HN

I like his take at the end:

> We converted a codebase that was adapted to Flow to TypeScript. This means that we obviously only found things that Flow can express but TypeScript can't. If the port had been the other way around, I'm sure we would have found things that TypeScript can infer/express better than Flow.

So he's aware of the conundrum, and, I hope, his readers too.

Concat doesn't mutate the original array, so there is no reason the output has to have the same value as the original array. Typescript has chosen to implement it this way, but it's not a limitation of the underlying JS method. Saying it's wrong because it's how Typescript has typed the function is circular reasoning.

Consider the same functionality exposed as a function, it would seem be fine to type this as:

   function concat(a: Array<A>, b: Array<B>): Array<A | B> { }

Also, you may consider

    [...leaves, ...aggregators]

Author here. I understand this example may be a little controversial. However, from my point of view, if I annotate my function as returning an `Array<MemsqlNode>` and `leaves.concat(aggregators)` can be cast to `Array<MemsqlNode>`, then I don't see why I shouldn't be able to return this in this function without verbose syntax or "tricks".

it's not a trick. Yes, it can be casted, but you should be explicit about it. AFAIK, the signature for the concat function is dependent on the array you're using it, so in this case, it's Array<Leaves>. Even though concat returns a new array, it is expecting the arguments to be of similar type. That's a "safe" behavior and something I would expect. Auto-casting to the return type (in this case) is something that I would not expect to happen, as it could be the source of errors.

Doing [].concat(Array<A>, Array<B>) is fine, because you did not specify the type of the array for the literal [], it's not a trick, in that case the type should be Array<any>, but it's casted to Array<MemsqlNode>.

Looking at the type definition, it is expecting the same type:

    /**                                                                                                                                                                                                                                                                                                                       
      * Combines two or more arrays.                                                                                                                                                                                                                                                                                          
      * @param items Additional items to add to the end of array1.                                                                                                                                                                                                                                                            
      */                                                                                                                                                                                                                                                                                                                      
    concat(...items: ConcatArray<T>[]): T[];                                                                                                                                                                                                                                                                                  
    /**                                                                                                                                                                                                                                                                                                                       
      * Combines two or more arrays.                                                                                                                                                                                                                                                                                          
      * @param items Additional items to add to the end of array1.                                                                                                                                                                                                                                                            
      */                                                                                                                                                                                                                                                                                                                      
    concat(...items: (T | ConcatArray<T>)[]): T[];

That's typescript's type definition.

Flow recognizes that concat returns a new array, so it specifies that calling concat on an Array<T> with an Array<S> as an argument returns an Array<T|S>. Their actual definition is:

    declare class $ReadOnlyArray<+T> {
        // concat creates a new array
        concat<S, Item: $ReadOnlyArray<S> | S>(...items: Array<Item>): Array<T | S>;

Personally, I'm ok with that and think it is useful. Although the return value could be an array of any type more general than T|S and the types of the argument arrays could vary. (i.e. Array<T>.concat(Array<S1>,Array<S2>,...): Array<? extends (T|S1|S2)>) but I don't know if that can be expressed in flow.

I don't think either gets it perfect, but flow is trying to at least capture the fact that the return value can't be an array of a type that is disjoint from T|S.

In practice, I've been impressed by the level of detail of information that is captured and propagated by flow's type checker.

Right but isn't that effectively changing the runtime code (thus making it a trick) in order to fix a type error? I don't know if the performance of `a.concat(b)` is the same as `[].concat(a, b)`.

It's not a type error. I think the error is in your definition of a function and types like that. I would've defined your types like in this example[0]. However, I don't know enough about your codebase or why you're doing this.

[0]: http://www.typescriptlang.org/play/#src=type%20NodeType%20%3...

The argument is that you have a potential type error in `a.concat(b)` that you hadn't considered before in that code: If `a` is defined everywhere as Array<Leaf> and you push() something to it that is not a Leaf you potentially broke one of your own invariants somewhere else that uses `a`. In JS concat happens in place and is a mass push, so the same general concept holds.

`concat` doesn't happen in place, it builds a new array:

    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat

Interesting. Then that should be a fixable bug in TS' lib files then.

There's an issue mentioning it with a PR Welcome sign up: https://github.com/Microsoft/TypeScript/issues/27545

Ah, and there is at least one PR specifically for it: https://github.com/Microsoft/TypeScript/pull/25716

True, but does not mean the type of the array that should be built should be of different types. I think the sane definition by Typescript is that it should be of the same type: that's why it's an instance method and not a static one. If this was a static method (Array.concat(a, b)), I would agree that would be a good assumption, that the type of the resulting array should be union of the types of the arguments.

If the return type was Array<Leaf | Aggregator>, wouldn't TypeScript be able to infer the right type U in Array<T>.concat => Array<U> ?

No, because JS concat is an in place operation, Typescript models it as Array<T>.concat() (intentionally leaves it "narrow") rather than Array<T>.concat<U extends T>(). In this case it is the input type that potentially would need to change so that the left hand side was Array<Leaf | Aggregator>.

Or as others point out, array spread does widen unlike concat (because it isn't an in-place operation).

Apparently concat does not operate in place, my mistake. I've been coding in TS for so long I assumed its definition was correct.

https://news.ycombinator.com/item?id=18907771

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Refe...

Reply on news.ycombinator.com