3 ms·

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

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)`.

funkaster 8y ago
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%3D%20%22LEAF%22%20%7C%20%22AGGREGATOR%22%3B%0D%0A%0D%0Atype%20MemsqlNode%20%3D%20%7B%0D%0A%20%20host%3A%20string%3B%0D%0A%20%20port%3A%20number%3B%0D%0A%20%20type%3A%20NodeType%3B%0D%0A%7D%0D%0A%0D%0Atype%20Leaf%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22LEAF%22%3B%0D%0A%7D%0D%0A%0D%0Atype%20Aggregator%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22AGGREGATOR%22%3B%0D%0A%7D%0D%0A%0D%0Afunction%20f(leaves%3A%20Array%3CLeaf%3E%2C%20aggregators%3A%20Array%3CAggregator%3E)%3A%20Array%3CMemsqlNode%3E%20%7B%0D%0A%20%20return%20(leaves%20as%20Array%3CMemsqlNode%3E).concat(aggregators)%3B%0D%0A%7D%0D%0A http://www.typescriptlang.org/play/#src=type%20NodeType%20%3...
WorldMaker 8y ago
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.
- dunham 8y ago
  `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
  - WorldMaker 8y ago
    Interesting. Then that should be a fixable bug in TS' lib files then.
    - WorldMaker 8y ago
      There's an issue mentioning it with a PR Welcome sign up: https://github.com/Microsoft/TypeScript/issues/27545 https://github.com/Microsoft/TypeScript/issues/27545
      - WorldMaker 8y ago
        Ah, and there is at least one PR specifically for it: https://github.com/Microsoft/TypeScript/pull/25716 https://github.com/Microsoft/TypeScript/pull/25716
  - funkaster 8y ago
    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.

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)`.

funkaster 8y ago

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%3D%20%22LEAF%22%20%7C%20%22AGGREGATOR%22%3B%0D%0A%0D%0Atype%20MemsqlNode%20%3D%20%7B%0D%0A%20%20host%3A%20string%3B%0D%0A%20%20port%3A%20number%3B%0D%0A%20%20type%3A%20NodeType%3B%0D%0A%7D%0D%0A%0D%0Atype%20Leaf%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22LEAF%22%3B%0D%0A%7D%0D%0A%0D%0Atype%20Aggregator%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22AGGREGATOR%22%3B%0D%0A%7D%0D%0A%0D%0Afunction%20f(leaves%3A%20Array%3CLeaf%3E%2C%20aggregators%3A%20Array%3CAggregator%3E)%3A%20Array%3CMemsqlNode%3E%20%7B%0D%0A%20%20return%20(leaves%20as%20Array%3CMemsqlNode%3E).concat(aggregators)%3B%0D%0A%7D%0D%0A http://www.typescriptlang.org/play/#src=type%20NodeType%20%3...

WorldMaker 8y ago

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.

dunham 8y ago
`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
- WorldMaker 8y ago
  Interesting. Then that should be a fixable bug in TS' lib files then.
  - WorldMaker 8y ago
    There's an issue mentioning it with a PR Welcome sign up: https://github.com/Microsoft/TypeScript/issues/27545 https://github.com/Microsoft/TypeScript/issues/27545
    - WorldMaker 8y ago
      Ah, and there is at least one PR specifically for it: https://github.com/Microsoft/TypeScript/pull/25716 https://github.com/Microsoft/TypeScript/pull/25716
- funkaster 8y ago
  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.