refactor(helpers)!: remove default value from arrayElement

[xDivisionByZerox]

Closes #219.

[codecov]

Codecov Report

Merging #2045 (cff4a33) into next (9161a06) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #2045   +/-   ##
=======================================
  Coverage   99.61%   99.61%           
=======================================
  Files        2535     2535           
  Lines      242221   242231   +10     
  Branches     1291     1299    +8     
=======================================
+ Hits       241283   241303   +20     
+ Misses        911      901   -10     
  Partials       27       27           

Impacted Files	Coverage Δ
src/modules/helpers/index.ts	99.06% <100.00%> (+<0.01%)	⬆️

... and 2 files with indirect coverage changes

[matthewmayer]

regression:

faker.helpers.arrayElement(["foo","bar",undefined]) 

now throws an error 1/3 of the time.

[Shinigami92]

regression:

faker.helpers.arrayElement(["foo","bar",undefined]) 

now throws an error 1/3 of the time.

Interesting case we might now have thought about

I think we can use something like this:

[xDivisionByZerox]

I think we can use something like this:

Or we simply check the array length on input and throw? Why get that complex...^^

If someone provides an array with undefined this will be inferred by the generic and correctly type the return type.

[Shinigami92]

I think we can use something like this:

Or we simply check the array length on input and throw? Why get that complex...^^

If someone provides an array with undefined this will be inferred by the generic and correctly type the return type.

Because of

faker/src/modules/helpers/index.ts

Lines 801 to 803 in 99c16e4

	const value = array[index];
	if (value === undefined) {
	throw new FakerError('Cannot get value from empty set.');

[matthewmayer]

I think we can use something like this:

Or we simply check the array length on input and throw? Why get that complex...^^
If someone provides an array with undefined this will be inferred by the generic and correctly type the return type.

Because of

faker/src/modules/helpers/index.ts

Lines 801 to 803 in 99c16e4

	const value = array[index];
	if (value === undefined) {
	throw new FakerError('Cannot get value from empty set.');

can't you just change the condition for throwing from

const value = array[index];
    if (value === undefined) {
      throw new FakerError('Cannot get value from empty set.');
    }

to

    if (array.length === 0) {
      throw new FakerError('Cannot get value from empty set.');
    }
const value = array[index];

[matthewmayer]

To clarify, in Faker 7, the behavior was:

> faker.helpers.arrayElement([])
undefined
> faker.helpers.arrayElement()
'b'

Do we want it to now throw in both these cases (array is empty, array is undefined)?

[xDivisionByZerox]

can't you just change the condition for throwing from

const value = array[index];
    if (value === undefined) {
      throw new FakerError('Cannot get value from empty set.');
    }

to

    if (array.length === 0) {
      throw new FakerError('Cannot get value from empty set.');
    }
const value = array[index];

Yes and no. This is precisely what I was stating in my previous comment. But we have to keep in n mind that this will cause trouble in later versions if we decide to activate more strict typescript features. I'm talking about the noUncheckedIndexedAccess rule.

Do we want it to now throw in both these cases (array is empty, array is undefined)?

Yes, we do want that.
Providing no argument at all doesn't make sense to me. The method is in the helper module so the general consent of "every function has to be callable without arguments" does NOT apply here.
If you provide an empty array, typescript correctly infers that the return type is never, which basically means "This function never returns anything". This is correct as it throws an error instead f returning.

[matthewmayer]

Whether or not it makes sense, passing an undefined array happens in existing places in the code at the moment and ends up with "b".

We should add tests for various cases:

Undefined array
Empty array
Array which contains undefined as a possible value

[Shinigami92]

Whether or not it makes sense, passing an undefined array happens in existing places in the code at the moment and ends up with "b".

We should add tests for various cases:

Undefined array Empty array Array which contains undefined as a possible value

I highly assume that this is the reason why this PR is in "draft", but thanks for also thinking about it 🙂

please review again or approve

[ST-DDT]

please review again or approve

@Shinigami92 we are not in a hurry.

[Shinigami92]

please review again or approve

@Shinigami92 we are not in a hurry.

This was not related with being in a hurry in any case?!
I just wrote a message into the dismiss form that GitHub requires to dismiss stale review blockings

[ST-DDT]

This was not related with being in a hurry in any case?!
I just wrote a message into the dismiss form that GitHub requires to dismiss stale review blockings

Then please wait with dismissing their review, so they actually have time to review again.
If sufficient time has passed and we actually want to proceed with merging this it is still early enough to dismiss them then.