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

Typing of gender conflicts with common usage patterns #776

Closed
textbook opened this issue Apr 4, 2022 · 9 comments · Fixed by #1168
Closed

Typing of gender conflicts with common usage patterns #776

textbook opened this issue Apr 4, 2022 · 9 comments · Fixed by #1168
Labels
has workaround Workaround provided or linked m: person Something is referring to the person module needs documentation Documentations are needed s: needs decision Needs team/maintainer decision

Comments

@textbook
Copy link

textbook commented Apr 4, 2022

Describe the bug

Although in practice faker.name.firstName et al. can accept any value, their typing limits them to GenderType | undefined, which is currently 'female' | 'male' | 0 | 1 | undefined. The specific problem with this is that you can't use the return value from faker.name.gender(), whether or not you set binary to true, as the argument to the other methods.

Reproduction

This works fine in JavaScript:

faker.name.firstName(faker.name.gender());

but the exact same code in TypeScript gives:

Argument of type 'string' is not assignable to parameter of type 'GenderType | undefined'.ts (2345)

Additional Info

I found out about this via a Stack Overflow question: https://stackoverflow.com/q/71743871/3001761

@textbook textbook added the s: pending triage Pending Triage label Apr 4, 2022
@Shinigami92
Copy link
Member

Attention: faker.name.gender() is not designed to be used as an input value for gender-parameter.
faker.name.gender() can return any kind of string and also will not return Male or Female for locale !== en!

Instead of using faker.name.firstName(faker.name.gender()); you should use something like faker.name.firstName(faker.random.arrayElement(['female', 'male'] as GenderType[]));

But then you should start think about why using this in the first place 🤔
You could use just faker.name.firstName(); so without any parameter, so you will also get names that are potential non-fe/male. Otherwise it looks like you are forcefully trying to be non-inclusive?

So use functions like firstName only in three ways:

faker.name.firstName(); // Just return me a name
faker.name.firstName('female'); // I specifically target female (e.g. for a shop?)
faker.name.firstName('male'); // I specifically target male (e.g. for a shop?)

What we could do is to provide a helper function like faker.helpers.genderType(): GenderType. But even then my points above applies as a mindset and you can easily just use faker.random.arrayElement(['female', 'male']) yourself.

@Shinigami92 Shinigami92 added has workaround Workaround provided or linked s: needs decision Needs team/maintainer decision needs documentation Documentations are needed and removed s: pending triage Pending Triage labels Apr 5, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Apr 5, 2022

Otherwise it looks like you are forcefully trying to be non-inclusive?

I think this has nothing to do with being exclusive, this is just how forms in the web work.
If the form allows the selection of genders, there is a huge chance it does not support the same set of genders as we do (which might differ between locales anyway). Our gender() method is only useful for free text gender fields.

However, you pointed out correctly that the gender() method is not suitable for our name methods.

What we could do is to provide a helper function like faker.helpers.genderType(): GenderType.

IMO we should provide it, to allow the user to generate consistent data.
Please do not consider GenderType as ['female', 'male'] as they currently are, but as a way to generate consistent data.

In your head it should look like this:

const gender = genderType();
const firstName = firstName(gender);
const lastName = lastName(gender);

and not like in your example:

const gender = 'female';
const firstName = firstName('female');
const lastName = lastName('female');

Maybe to remove the current implementation bias from your/everyones head, we could add a generic/any (as in not specifically male or female/just give me a name) value to the enum and add a binary: boolean parameter to the genderType() method. That way the dev explicitly has the choice, whether they want/can deal with non-binary genders and how they do so.
Later we can add more values such as non-binary (as in explicitly both).

That being said, I would like to strictly limit the GenderType to as few values as possible, to avoid huge holes in the translations/non-en locales. Also we should have a well defined fallback strategy in case of missing locale files.
But both the additional gender types (beyond generic/any) and their fallbacks are out of scope of this issue.

IMO the genderType() method should be in the name module, because

  • It will be easier to search for/find
  • It is currently only useful in combination with names

For consistency we should add a way to map the gender type to a gender locale value.


TLDR:

  • We should have a genderType() in our name module
  • We should add a generic/any value to it to make it clear, that the enum/type is not about (binary-)genders. It is only meant to generate consistent data.

@ST-DDT ST-DDT added this to the v6.2 - New small features milestone Apr 5, 2022
@Shinigami92
Copy link
Member

Also some good point.
I would like to raise the concern that I understand that it could be useful to add in the name module, but doesn't make much sense in a design perspective, because currently you can expect that everything in the name module is based on locales.
I'm also not sure if something like faker.name.helpers.genderType(): GenderType would make sense, because this would be something totally new.
We should absolutely discuss this in the team so we can also see what the expectations of other team members would be.

I would not expect that a user's html form matches the API of faker and therefore they already need to write their mapper to be compatible to our API. So I would not like to assume that it should be just designed in that way.

I also wont like to expose a usable parameter like generic, maybe it's just the wrong term but maybe I think it is just too complex to be used. So a signature like firstName(gender?: 'female' | 'male' | 'non-binary'): string already suites all cases that come into my mind.
So IMO we would have:

faker.locale = 'de'
faker.name.firstName() // 'Gisela' or 'Herbert' or 'Toni'
faker.name.firstName('female') // 'Gisela'
faker.name.firstName('male') // 'Herbert'
faker.name.firstName('non-binary') // 'Toni'

So if we implement a function like getMeARandomGenderType(), it would break the author's assumption of just getting a female or male first name exclusively, and the author would need to fallback to the provided workaround 🤷
I'm personally totally against to implement a function in faker that returns just ['female', 'male'][randomIndex] in the long term perspective.

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 5, 2022

I just found out that it is possible to execute the following code:

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

const genders = Object.values(Gender);
const randomGender = faker.random.objectElement(Gender);

faker.name.firstName(genders[0]); // works
faker.name.firstName(randomGender); // works

@ST-DDT
Copy link
Member

ST-DDT commented Apr 5, 2022

After consulting with @Shinigami92 , I think we should call the new value any instead of generic.
This value is intentionally added to replace undefined and be cleanly passable to/returnable from a method.

type GenderType = 'any' | 'female' | 'male';
genderType(onlyBinary: boolean = false): GenderType;
firstName(gender: GenderType = 'any')

This also allows more explicit calls like firstName('any').

@Shinigami92
Copy link
Member

We could also provide export const GENDER_TYPES: readonly GenderType[] as a helper for iterating over all possible input parameters.

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 5, 2022

We had a HUGE internal and long conversation and we may want to switch GenderType to be 'femme' | 'masc' | 'non-gendered' or something similar.
This would have the advantage that you do not misinterpret e.g. Female from faker.name.gender() with the input parameter femme.

We are also currently asking in some communities for feedback and so it could take a while until we proceed further with stuff around GenderType

Edit: Reference-Link: https://twitter.com/_jessicasachs/status/1511375258548379662

@ST-DDT
Copy link
Member

ST-DDT commented Aug 19, 2022

@Shinigami92 I dont consider this fixed yet.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2022

Fixed by #1289

@ST-DDT ST-DDT closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has workaround Workaround provided or linked m: person Something is referring to the person module needs documentation Documentations are needed s: needs decision Needs team/maintainer decision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants