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

Incorrect behavior of name.firstName() function #547

Closed
JoCat opened this issue Feb 23, 2022 · 14 comments · Fixed by #1610
Closed

Incorrect behavior of name.firstName() function #547

JoCat opened this issue Feb 23, 2022 · 14 comments · Fixed by #1610
Labels
breaking change Cannot be merged when next version is not a major release c: bug Something isn't working has workaround Workaround provided or linked m: person Something is referring to the person module s: accepted Accepted feature / Confirmed bug

Comments

@JoCat
Copy link
Contributor

JoCat commented Feb 23, 2022

Describe the bug

When using the faker.name.firstName() function with a non-standard locale and without passing a gender value to the function, the name is still displayed in English.

Reproduction

For example:

import { faker } from '@faker-js/faker';

faker.locale = 'ru'; // Set another locale

const incorrectFirstName = faker.name.firstName(); // Jack (incorrect)
const correctFirstName = faker.name.firstName(0); // Евгений (correct)
const correctFirstName2 = faker.name.firstName(1); // Жанна (correct)

demo: https://stackblitz.com/edit/faker-js-demo-6fb7x2?file=index.ts

Additional Info

No response

@JoCat JoCat added the s: pending triage Pending Triage label Feb 23, 2022
@Shinigami92
Copy link
Member

Yeah, thx, we already know that the name functions are really buggy 🙁
We will target this as one of the top prios when starting working on v6.1

@Shinigami92 Shinigami92 added c: bug Something isn't working and removed s: pending triage Pending Triage labels Feb 23, 2022
@Shinigami92 Shinigami92 added this to the v6.1 - First bugfixes milestone Feb 23, 2022
@JoCat
Copy link
Contributor Author

JoCat commented Feb 23, 2022

thanks, maybe there is a problem in the code here:

faker/src/name.ts

Lines 38 to 65 in 03c3d11

if (typeof gender === 'string') {
if (gender.toLowerCase() === 'male') {
gender = 0;
} else if (gender.toLowerCase() === 'female') {
gender = 1;
}
}
if (typeof gender !== 'number') {
if (typeof this.faker.definitions.name.first_name === 'undefined') {
gender = this.faker.datatype.number(1);
} else {
// Fall back to non-gendered names if they exist and gender wasn't specified
return this.faker.random.arrayElement(
this.faker.definitions.name.first_name
);
}
}
if (gender === 0) {
return this.faker.random.arrayElement(
this.faker.definitions.name.male_first_name
);
} else {
return this.faker.random.arrayElement(
this.faker.definitions.name.female_first_name
);
}
}

at least it differs from what is in the lastName and middleName functions, which work correctly

@Shinigami92
Copy link
Member

I mean... that is quite the whole function except of 2-3 lines 😉
Maybe also have a look into #440

@Shinigami92
Copy link
Member

Needs re-evaluation after #644 was merged

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Mar 23, 2022
@JoCat
Copy link
Contributor Author

JoCat commented Mar 23, 2022

Now displays Deprecation Warning
image

@Shinigami92
Copy link
Member

Yes, that is freshly implemented by us 🙂
Please do not use 0 or 1 as parameter, but 'male' or 'female'

@JoCat
Copy link
Contributor Author

JoCat commented Mar 23, 2022

Yes, that is freshly implemented by us 🙂
Please do not use 0 or 1 as parameter, but 'male' or 'female'

yes, didn't notice
It turns out the bug is still there

image

@Shinigami92 Shinigami92 self-assigned this Mar 23, 2022
@Shinigami92 Shinigami92 removed the s: on hold Blocked by something or frozen to avoid conflicts label Mar 24, 2022
@Shinigami92 Shinigami92 removed their assignment Mar 24, 2022
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Mar 24, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 24, 2022

This issue is more complex then it looks like.

The basics:

  • female is defined in ru
  • male is defined in ru
  • generic is defined in en (fallback language)
  • If generic is not defined, we will use female or male randomly as fallback.

The possible solutions:

  1. Create/Configure a faker instance without the en fallback (Suggested workaround for now)
  2. Check whether generic is defined in the same language as female and male, if not treat it as not defined
  • This cannot be done via faker.locales[faker.locale].name.xyz because we wish to add support for multiple fallback languages in v7 (en_AU_ocker -> en_AU -> en).
    Thus we would have to add a new "trace" property similar to definitions, that returns the source locale of a property faker.trace.name.firstname => 'en'
  1. Create/Generate the generic source file automatically, if it is missing (pnpm run generate:locales)

The generic file may contain entries for non binary genders.

All name methods (prefix, suffix, firstname, middlename, lastname, ...) should behave the same in this regard.

@ST-DDT ST-DDT added the has workaround Workaround provided or linked label Mar 24, 2022
@JoCat
Copy link
Contributor Author

JoCat commented Mar 24, 2022

yes, it looks like when specifying localeFallback in ru, the code works correctly
https://stackblitz.com/edit/faker-js-demo-hxvbec?file=index.ts
although this is more like a temporary fix, and should not be taken as a full solution to the problem

If I understand correctly, the third option (indicate male and female names (surnames, patronymics) in the general array) is acceptable

@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2022

Team decision

Change data-structure:

name.female_first_name ->person.first_name.female

Example:

type Gendered = {
  female: string[]
  male: string[]
  generic: string[]
};

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision s: accepted Accepted feature / Confirmed bug breaking change Cannot be merged when next version is not a major release and removed s: needs decision Needs team/maintainer decision labels Sep 8, 2022
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Nov 15, 2022
@matthewmayer
Copy link
Contributor

Maybe I'm missing somethng, but isn't the simplest solution to add a first_name.ts file to locales which are missing one which simply concatenates the existing male_first_name and female_first_name lists?

Had a go at this in #1610

@ST-DDT
Copy link
Member

ST-DDT commented Nov 27, 2022

If you add a new locale or change the locale config (for most of the parts in person), then you will encounter the bug again.
Changing the structure prevents that because then you have to either configure the entire block correctly or you will get compile time errors. You can either not define anything, define only generic, define only male and female or define all.

@ST-DDT
Copy link
Member

ST-DDT commented Dec 21, 2022

The effect of the bug is fixed. The implementation/structure should still be adjusted accordingly.

@ST-DDT
Copy link
Member

ST-DDT commented Dec 21, 2022

Further changes tracked via #1677

@ST-DDT ST-DDT closed this as completed Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: bug Something isn't working has workaround Workaround provided or linked m: person Something is referring to the person module s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
5 participants