The `first` and `last` methods should have `undefined` in their return type signature

[rakeshpai]

If the array is empty, first and last return undefined, but their type signatures say that they return an array item.

(This could be considered a breaking change.)

[selfrefactor]

Thanks for bringing this up. I'll check and get back to you.

[selfrefactor]

Not sure if you are correct as these test reveals the opposite:

  it('empty array', () => {
    const result = last([])
    result // $ExpectType undefined
  })

  it('empty array', () => {
    const result = head([])
    result // $ExpectType undefined
  })

Also, Rambda doesn't have first as a method.

[rakeshpai]

Ah, sorry about the first. The issue I ran into was with last anyway, and I just assumed that first would exist and suffer from the same issue.

I've created an example here that demos the problem I'm talking about: https://codesandbox.io/s/shy-tdd-2dcey?file=/src/index.ts In the lines that use rambda's last, notice I don't get a type error, leading to unsafe code. In the quick-and-dirty version of last I created below that, there's a type error in its usage as it should be, IMHO.

[selfrefactor]

Thanks for the example. You can check the same example with Ramda and I am sure you will get the same result as with Rambda. I feel that your example code has problem as you promise to Typescript that you have a list of strings and then you abandon the same promise by assigning it to empty array.

I am not closing the issue, as it is a correct use case. After all, Typescript allows you to do that. I will try to come back with more info.

[selfrefactor]

So, I tried to introduce better typings with export function head<T>(input: T[]): T extends [] ? undefined : T; but it didn't help. I will close the issue as I see no problem with the types. Please, feel free to comment further if you have something to add.

[rakeshpai]

Am empty array is a valid array of strings. One way to think of it is say I .push to the array after declaring it, and I want the operation to be type-safe (i.e. I want to ensure that numbers aren't .pushed), I'd give the empty array a type annotation of string[] even though I'd want it to be empty in the start.

const arr: string[] = [];
arr.push('hello');
arr.push('world');
arr.push(42); // type error

Now, if the .push was conditional, and that conditional code-path wasn't hit, the last would be operating on an empty array, and the result would be undefined. So, a forced undefined check would be a good idea.

const arr: string[] = [];
if (/* something that evaluates to false */) {
  arr.push('hello');
}
last(arr); // undefined

The example above is admittedly contrived, but a slightly more realistic example could be as follows:

const foo = ['hello', 'world'].filter(x => x.startsWith('abc')); // results in an empty array of strings
last(foo); // Is `undefined`, but type signature says `string`

Hope this helps.

[rakeshpai]

Sorry for the spam, but to add to the last example, there are several ways of ending up with an empty array of a particular type:

const arr1 = ['foo', 'bar'].filter(x => x.startsWith('a')); // same as example above, empty array of strings
const arr2 = ''.split('/'); // empty array of strings
const arr3 = [10,20].filter(x => x < 5); // variation of arr1, empty array of numbers
const arr4 = Object.keys({}); // empty array of strings
const arr5 = [...new Set<number>()]; // empty array of numbers
const arr6 = [...document.querySelectorAll('.doesnt-exist')]; // empty array of Elements
const arr7 = [new Date('1970-01-01')].filter(x => x > new Date()); // empty array of Dates

In the examples above, I'm not tricking TS by giving it an unfavourable type annotation - they are strongly typed by type inference, and yet empty arrays. head or last on these arrays should typecheck to T | undefined, so that I'm forced to check if I've received a value or undefined.

[selfrefactor]

Thanks for the clarification. In this case, I am reopening the ticket and I'll get back to you.

[selfrefactor]

I guess it is a question about the Typescript version.

This is green test with Typescript 4.1.5

 it('empty array', () => {
    const arr: string[] = [];
    const l = head(arr);
    // $ExpectError
    l.toLowerCase();
  })

Do you know what version is used in the playground?

Also you can notice in the next references that there are not many other ways to have Head definition:

• https://github.com/krzkaczor/ts-essentials/blob/master/lib/types.ts
• https://github.com/iter-tools/iter-tools/blob/trunk/src/methods/array-last-or.d.ts

[rakeshpai]

Sorry, I didn't have the time to understand the build process, but I managed to get the types to work with a simple fix. https://github.com/selfrefactor/rambda/pull/610 Feel free to reject this PR itself for not complying with the repo rules, but I just wanted to convey what the fix could look like.

Interesting, head is already typed correctly, and doesn't require any fixes. So, the changes in the PR only brings the definition of last to be in line with head.

Note, if you do decide to keep the changes I've made, this should be considered a breaking change.

[rakeshpai]

Also, to your point, this doesn't have to do with the TS version. I think the error in the original/current type definition is that the empty array case is explicitly typed:

export function last(emptyList: []): undefined;

However, (and I'm guessing here) TS can't know if this type signature should be used if the provided array is the result of some runtime computation, since TS has already stepped out of the picture by then. So, for the list of arrays I provided above which already have an associated type, TS will use the next type definition:

export function last<T extends any>(list: T[]): T;

which results in an always T result without an undefined.

Furthermore, the test only checked the explicit empty array condition:

  it('empty array', () => {
    const result = last([])
    result // $ExpectType undefined
  })

But if we changed the test to provide an empty array created at runtime, the test would break:

  const result = last(['hello', 'world'].filter(x => x.startsWith('abc')));
  result // $ExpectType undefined <- This would break

The solution is to always allow for the possibility that the array is empty, and force a possible undefined, like is being done with head.

export function last<T extends any>(list: T[]): T | undefined;

[selfrefactor]

Thanks; I will look into your changes and get back to you. One thing I have as comment is that such TS definition change is not a braking change for Rambda.

[rakeshpai]

👍 Your call, but the reason I was saying this is a breaking change is because if people upgrade, they will get type errors if they aren't already doing an undefined check in their code for the value returned from last(...).

[selfrefactor]

I understand your point but braking change usually require major bump and I use that when indeed there is significant change.

Do you have any comment on the 2 links I shared above? I ask, as they prove that there are not many ways to define Head in Typescript.

[rakeshpai]

In the first link, the Head type seems like it's a utility type, not a function type definition. https://github.com/krzkaczor/ts-essentials#functional-type-essentials This is an awesome trick, but only works at compile time. When dealing with runtime arrays, this can't work, since we don't know what values the array would have. Doesn't diminish its compile time usefulness though - it's pretty neat. In any case, this isn't super-relevant for our use, since it's not a function type definition.

Regarding the second link, I guess the type definition for first in this file https://github.com/iter-tools/iter-tools/blob/trunk/src/impls/array-first/array-first.d.ts which returns T | undefined, which I agree with. Ditto for last https://github.com/iter-tools/iter-tools/blob/trunk/src/impls/array-last/array-last.d.ts which also returns T | undefined.

For a function type definition, we'll have to stick to the second approach of T | undefined, since the array might be constructed at runtime, and could be empty. Unfortunately, this does add the annoyance that the consumer of these functions will have to add a check for undefined before accessing the value, but it's a case of 'better safe than sorry'. The user could use a ! if they're sure they won't have an empty array (which brings about the other annoyance that @typescript-eslint/recommended will complain about it), but I think this kind of explicit `I know what I'm doing here, step out of the way' is better than the unchecked runtime error that can happen.

As an example of this behaviour in the core TS lib, standard functions like array.find which has a similar semantic of 'pick one element from an array' returns T | undefined. https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.core.d.ts#L12

[selfrefactor]

I am unsure if removing typing for empty array is beneficial.

This is the current state of typings test with your suggestion:

import {last} from 'rambda'

describe('R.last', () => {
  it('string', () => {
    const result = last('foo')
    result // $ExpectType string
  })

  it('array', () => {
    const result = last([1, 2, 3])
    result // $ExpectType number | undefined
  })

  it('empty array - case 1', () => {
    const result = last([])
    result // $ExpectType undefined
  })
  it('empty array - case 2', () => {
    const list = ['foo', 'bar'].filter(x => x.startsWith('a'));
    const result = last(list)
    result // $ExpectType number | undefined
  })
})

Let me know if this works for you. If so, I will publish it with the new version.

[selfrefactor]

I am adding one more reference for possible head typings, but as the other cases, it didn't helped me much:

https://github.com/ronami/meta-typing/blob/master/src/head/index.d.ts

[selfrefactor]

I am closing the issue as the fix is released with version 7.0.0.