Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(helpers)!: remove default value from arrayElement #2045

Merged
merged 26 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4645f8f
refactor(helpers): remove default value from arrayElement
xDivisionByZerox Apr 13, 2023
80e2d59
docs(helpers): add `@throw` to arrayElement
xDivisionByZerox Apr 13, 2023
99c16e4
refactor(helpers): remove default value from arrayElements
xDivisionByZerox Apr 13, 2023
c560147
refactor(helpers): allow arrays with undefined values
xDivisionByZerox Apr 14, 2023
cbd1731
docs: breaking array element behavior
xDivisionByZerox Apr 14, 2023
944e73b
refactor(helpers): array element error
xDivisionByZerox Apr 16, 2023
399d4b2
docs: add examples to upgrading
xDivisionByZerox Apr 16, 2023
7e305d1
chore: update todo comment
xDivisionByZerox Apr 16, 2023
182f13f
Merge branch 'next' into refactor/helpers/array-elemt-remove-default-…
ST-DDT Apr 16, 2023
7b4ba2f
Apply suggestions from code review
xDivisionByZerox Apr 16, 2023
909a1e6
Apply suggestions from code review 2
xDivisionByZerox Apr 16, 2023
8bdd01f
docs: more explicit phrasing
xDivisionByZerox Apr 17, 2023
1e453a0
refactor(helpers): throw on undefined arguments
xDivisionByZerox Apr 20, 2023
2f9050e
Merge branch 'next' into refactor/helpers/array-elemt-remove-default-…
xDivisionByZerox Apr 20, 2023
0257d58
docs(helpers): rephrase JSDocs, again
xDivisionByZerox Apr 20, 2023
aa3ae3b
refactor(helpers): throw FakerError
xDivisionByZerox Apr 20, 2023
e5e8f95
test(helpers): add call on no arguments
xDivisionByZerox Apr 20, 2023
f721e8d
test(helpers): revert specific error in nullish value case
xDivisionByZerox Apr 20, 2023
0be6ae2
chore: add todo's
xDivisionByZerox Apr 20, 2023
7bc02c6
chore: replace `@ts-ignore` with `@ts-expect-error`
xDivisionByZerox Apr 20, 2023
39dd31a
test(helpers): arrayElements with nullish values
xDivisionByZerox Apr 20, 2023
4310b36
Merge branch 'next' into refactor/helpers/array-elemt-remove-default-…
ST-DDT Apr 20, 2023
14f8c1a
Apply suggestions from code review
xDivisionByZerox Apr 20, 2023
3285d1d
Apply suggestions from code review
Shinigami92 Apr 21, 2023
06b5087
Merge branch 'next' into refactor/helpers/array-elemt-remove-default-…
ST-DDT Apr 21, 2023
cff4a33
Merge branch 'next' into refactor/helpers/array-elemt-remove-default-…
ST-DDT Apr 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions docs/guide/upgrading.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,50 @@ For more information refer to our [Localization Guide](localization).

The `faker.location.zipCodeByState` method has been deprecated, but will also now throw an error if the current locale does not have a `postcode_by_state` definition.

### Methods will throw on empty data set inputs

The methods `faker.helpers.arrayElement` and `faker.helpers.arrayElements` previously defaulted the `array` argument to a simple string array if none was provided.
This behavior is no longer supported, as the default value has been removed.
You are now required to provide an argument.

Additionally, when passing in an empty array argument (`[]`), the functions previously returned `undefined`.
This behavior violated the expected return type of the method.
The methods will now throw an `FakerError` instead.

The same thing happens now if you provide an empty object `{}` to `faker.helpers.objectKey` or `faker.helpers.objectValue`.

xDivisionByZerox marked this conversation as resolved.
Show resolved Hide resolved
**Old**

```ts
const allTags = ['dogs', 'cats', 'fish', 'horses', 'sheep'];
const tags = faker.helpers.arrayElements(allTags, { min: 0, max: 3 });
// `tags` might be an empty array which was no problem in v7
const featuredTag = faker.helpers.arrayElement(tags);
// `featureTag` will be typed as `string` but could actually be `undefined`
```

**New**

```ts
const allTags = ['dogs', 'cats', 'fish', 'horses', 'sheep'];
const tags = faker.helpers.arrayElements(allTags, { min: 0, max: 3 });
// `tags` might be an empty array which will throw in v8
const featuredTag =
tags.length === 0 ? undefined : faker.helpers.arrayElement(tags);
// `featureTag` has to be explicitly set to `undefined` on your side

// OR

const allTags = ['dogs', 'cats', 'fish', 'horses', 'sheep'];
const tags = faker.helpers.arrayElements(allTags, { min: 0, max: 3 });
let featuredTag: string | undefined;
try {
featuredTag = faker.helpers.arrayElement(post.tags);
} catch (e) {
// handle error and do something special
}
```

### Other deprecated methods removed/replaced

| Old method | New method |
Expand Down
36 changes: 23 additions & 13 deletions src/modules/helpers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -794,18 +794,27 @@ export class HelpersModule {
*
* @template T The type of the elements to pick from.
*
* @param array Array to pick the value from.
* @param array The array to pick the value from.
*
* @throws If the given array is empty.
*
* @example
* faker.helpers.arrayElement(['cat', 'dog', 'mouse']) // 'dog'
*
* @since 6.3.0
*/
arrayElement<T = string>(
// TODO @Shinigami92 2022-04-30: We want to remove this default value, but currently it's not possible because some definitions could be empty
// See https://github.com/faker-js/faker/issues/893
array: ReadonlyArray<T> = ['a', 'b', 'c'] as unknown as ReadonlyArray<T>
): T {
arrayElement<T>(array: ReadonlyArray<T>): T {
ST-DDT marked this conversation as resolved.
Show resolved Hide resolved
// TODO @xDivisionByZerox 2023-04-20: Remove in v9
if (array == null) {
xDivisionByZerox marked this conversation as resolved.
Show resolved Hide resolved
throw new FakerError(
'Calling `faker.helpers.arrayElement()` without arguments is no longer supported.'
);
}

if (array.length === 0) {
throw new FakerError('Cannot get value from empty dataset.');
}

const index =
array.length > 1 ? this.faker.number.int({ max: array.length - 1 }) : 0;

Expand Down Expand Up @@ -891,9 +900,7 @@ export class HelpersModule {
* @since 6.3.0
*/
arrayElements<T>(
ST-DDT marked this conversation as resolved.
Show resolved Hide resolved
// TODO @Shinigami92 2022-04-30: We want to remove this default value, but currently it's not possible because some definitions could be empty
// See https://github.com/faker-js/faker/issues/893
array: ReadonlyArray<T> = ['a', 'b', 'c'] as unknown as ReadonlyArray<T>,
array: ReadonlyArray<T>,
count?:
| number
| {
Expand All @@ -907,6 +914,13 @@ export class HelpersModule {
max: number;
}
): T[] {
// TODO @xDivisionByZerox 2023-04-20: Remove in v9
if (array == null) {
xDivisionByZerox marked this conversation as resolved.
Show resolved Hide resolved
throw new FakerError(
'Calling `faker.helpers.arrayElements()` without arguments is no longer supported.'
);
}

if (array.length === 0) {
return [];
}
Expand Down Expand Up @@ -1117,10 +1131,6 @@ export class HelpersModule {
fake(pattern: string | string[]): string {
if (Array.isArray(pattern)) {
pattern = this.arrayElement(pattern);
// TODO @ST-DDT 2022-10-15: Remove this check after we fail in `arrayElement` when the array is empty
if (pattern == null) {
throw new FakerError('Array of pattern strings cannot be empty.');
}
}

// find first matching {{ and }}
Expand Down
27 changes: 0 additions & 27 deletions test/__snapshots__/helpers.spec.ts.snap
Original file line number Diff line number Diff line change
@@ -1,16 +1,7 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html

exports[`helpers > 42 > arrayElement > noArgs 1`] = `"b"`;

exports[`helpers > 42 > arrayElement > with array 1`] = `"o"`;

exports[`helpers > 42 > arrayElements > noArgs 1`] = `
[
"b",
"c",
]
`;

exports[`helpers > 42 > arrayElements > with array 1`] = `
[
"r",
Expand Down Expand Up @@ -226,18 +217,8 @@ exports[`helpers > 42 > weightedArrayElement > with array 1`] = `"sunny"`;

exports[`helpers > 42 > weightedArrayElement > with array with percentages 1`] = `"sunny"`;

exports[`helpers > 1211 > arrayElement > noArgs 1`] = `"c"`;

exports[`helpers > 1211 > arrayElement > with array 1`] = `"!"`;

exports[`helpers > 1211 > arrayElements > noArgs 1`] = `
[
"a",
"c",
"b",
]
`;

exports[`helpers > 1211 > arrayElements > with array 1`] = `
[
"d",
Expand Down Expand Up @@ -469,16 +450,8 @@ exports[`helpers > 1211 > weightedArrayElement > with array 1`] = `"snowy"`;

exports[`helpers > 1211 > weightedArrayElement > with array with percentages 1`] = `"snowy"`;

exports[`helpers > 1337 > arrayElement > noArgs 1`] = `"a"`;

exports[`helpers > 1337 > arrayElement > with array 1`] = `"l"`;

exports[`helpers > 1337 > arrayElements > noArgs 1`] = `
[
"b",
]
`;

exports[`helpers > 1337 > arrayElements > with array 1`] = `
[
"l",
Expand Down
6 changes: 0 additions & 6 deletions test/__snapshots__/location.spec.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,6 @@ exports[`location > 42 > streetAddress > with boolean 1`] = `"7917 Miller Park"`

exports[`location > 42 > streetAddress > with useFullAddress options 1`] = `"7917 Miller Park Apt. 410"`;

exports[`location > 42 > streetName 1`] = `"b"`;

exports[`location > 42 > timeZone 1`] = `"America/North_Dakota/New_Salem"`;

exports[`location > 42 > zipCode > noArgs 1`] = `"79177"`;
Expand Down Expand Up @@ -296,8 +294,6 @@ exports[`location > 1211 > streetAddress > with boolean 1`] = `"487 Breana Wells

exports[`location > 1211 > streetAddress > with useFullAddress options 1`] = `"487 Breana Wells Apt. 616"`;

exports[`location > 1211 > streetName 1`] = `"c"`;

exports[`location > 1211 > timeZone 1`] = `"Pacific/Fiji"`;

exports[`location > 1211 > zipCode > noArgs 1`] = `"48721-9061"`;
Expand Down Expand Up @@ -450,8 +446,6 @@ exports[`location > 1337 > streetAddress > with boolean 1`] = `"51225 Alexys Gat

exports[`location > 1337 > streetAddress > with useFullAddress options 1`] = `"51225 Alexys Gateway Apt. 552"`;

exports[`location > 1337 > streetName 1`] = `"a"`;

exports[`location > 1337 > timeZone 1`] = `"America/Guatemala"`;

exports[`location > 1337 > zipCode > noArgs 1`] = `"51225"`;
Expand Down
46 changes: 46 additions & 0 deletions test/all_functional.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,50 @@ const BROKEN_LOCALE_METHODS = {
companySuffix: ['az'],
},
location: {
city: ['th'],
state: ['az', 'nb_NO', 'sk'],
stateAbbr: ['sk'],
streetName: [
'af_ZA',
'ar',
'dv',
'el',
'en',
'en_AU',
'en_BORK',
'en_CA',
'en_GB',
'en_GH',
'en_IE',
'en_IN',
'en_NG',
'en_US',
'en_ZA',
'es',
'fa',
'fi',
'fr',
'fr_BE',
'fr_CA',
'fr_CH',
'fr_LU',
'hu',
'hy',
'id_ID',
'it',
'ja',
'ne',
'nl',
'nl_BE',
'pl',
'pt_BR',
'pt_PT',
'ur',
'vi',
'zh_CN',
'zh_TW',
'zu_ZA',
],
},
random: {
locale: '*', // locale() has been pseudo removed
Expand All @@ -40,6 +82,10 @@ const BROKEN_LOCALE_METHODS = {
person: {
prefix: ['az', 'id_ID', 'ru', 'zh_CN', 'zh_TW'],
suffix: ['az', 'it', 'mk', 'pt_PT', 'ru'],
jobArea: ['ar', 'fr', 'fr_BE', 'fr_CA', 'fr_CH', 'fr_LU'],
jobDescriptor: ['ar', 'fr', 'fr_BE', 'fr_CA', 'fr_CH', 'fr_LU'],
jobTitle: ['ar', 'fr', 'fr_BE', 'fr_CA', 'fr_CH', 'fr_LU', 'ur'],
jobType: ['ur'],
},
} satisfies {
[module in keyof Faker]?: SkipConfig<Faker[module]>;
Expand Down
67 changes: 56 additions & 11 deletions test/helpers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('helpers', () => {
});

t.describe('arrayElement', (t) => {
t.it('noArgs').it('with array', 'Hello World!'.split(''));
t.it('with array', 'Hello World!'.split(''));
});

t.describe('enumValue', (t) => {
Expand Down Expand Up @@ -119,8 +119,7 @@ describe('helpers', () => {
});

t.describe('arrayElements', (t) => {
t.it('noArgs')
.it('with array', 'Hello World!'.split(''))
t.it('with array', 'Hello World!'.split(''))
.it('with array and count', 'Hello World!'.split(''), 3)
.it('with array and count range', 'Hello World!'.split(''), {
min: 1,
Expand Down Expand Up @@ -206,6 +205,30 @@ describe('helpers', () => {

expect(actual).toBe('hello');
});

it('should throw with no arguments', () => {
// @ts-expect-error: `arrayElement` without arguments is not supported in TypeScript
expect(() => faker.helpers.arrayElement()).toThrowError(
new FakerError(
'Calling `faker.helpers.arrayElement()` without arguments is no longer supported.'
)
);
});

it('should throw on an empty array', () => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
expect(() => faker.helpers.arrayElement([])).toThrowError(
new FakerError('Cannot get value from empty dataset.')
);
});

describe('should not throw on an array with nullish elements', () => {
it.each(['', 0, undefined, null, false])('%s', (nullishValue) => {
expect(() =>
faker.helpers.arrayElement([nullishValue])
).not.toThrowError();
});
});
ST-DDT marked this conversation as resolved.
Show resolved Hide resolved
});

describe('enumValue', () => {
Expand Down Expand Up @@ -464,6 +487,26 @@ describe('helpers', () => {
}
}
);

it('should throw with no arguments', () => {
// @ts-expect-error: `arrayElements` without arguments is not supported in TypeScript
expect(() => faker.helpers.arrayElements()).toThrowError(
new FakerError(
'Calling `faker.helpers.arrayElements()` without arguments is no longer supported.'
)
);
});

describe('should not throw on an array with nullish elements', () => {
it.each(['', 0, undefined, null, false])('%s', (nullishValue) => {
expect(() =>
faker.helpers.arrayElements(
[nullishValue, nullishValue, nullishValue],
2
)
).not.toThrowError();
});
});
});

describe('slugify()', () => {
Expand Down Expand Up @@ -867,9 +910,10 @@ describe('helpers', () => {
expect(Object.keys(testObject)).toContain(actual);
});

it('should return undefined if given object is empty', () => {
const actual = faker.helpers.objectKey({});
expect(actual).toBeUndefined();
it('should throw if given object is empty', () => {
expect(() => faker.helpers.objectKey({})).toThrowError(
new FakerError('Cannot get value from empty dataset.')
);
});
});

Expand All @@ -885,9 +929,10 @@ describe('helpers', () => {
expect(Object.values(testObject)).toContain(actual);
});

it('should return undefined if given object is empty', () => {
const actual = faker.helpers.objectValue({});
expect(actual).toBeUndefined();
it('should throw if given object is empty', () => {
expect(() => faker.helpers.objectValue({})).toThrowError(
new FakerError('Cannot get value from empty dataset.')
);
});
});

Expand Down Expand Up @@ -944,9 +989,9 @@ describe('helpers', () => {
expect(actual).toMatch(/^\d{5}$/);
});

it('does not allow empty array parameters', () => {
it('should throw with empty array parameters', () => {
expect(() => faker.helpers.fake([])).toThrowError(
new FakerError('Array of pattern strings cannot be empty.')
new FakerError('Cannot get value from empty dataset.')
);
});

Expand Down
5 changes: 4 additions & 1 deletion test/location.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ const NON_SEEDED_BASED_RUN = 5;

describe('location', () => {
seededTests(faker, 'location', (t) => {
t.itEach('street', 'streetName');
t.it('street');

// TODO @xDivisionByZerox 2023-04-16: add street name locale data to `en`
t.skip('streetName');

t.it('buildingNumber');

Expand Down