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

faker.lorem.word can return undefined despite being typed as string #1131

Closed
justinsmid opened this issue Jul 5, 2022 · 12 comments · Fixed by #1407
Closed

faker.lorem.word can return undefined despite being typed as string #1131

justinsmid opened this issue Jul 5, 2022 · 12 comments · Fixed by #1407
Assignees
Labels
c: bug Something isn't working help wanted Extra attention is needed m: lorem Something is referring to the lorem module

Comments

@justinsmid
Copy link

justinsmid commented Jul 5, 2022

Describe the bug

When calling faker.lorem.word with a length that doesn't have any matching words, it will return undefined while TypeScript thinks it's a string.

Reproduction

const nonexistentWord = faker.lorem.word(9999) // <-- typed as `string`
console.log(nonexistentWord) // <-- will output `undefined`

Additional Info

No response

@justinsmid justinsmid added the s: pending triage Pending Triage label Jul 5, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jul 5, 2022

What is your expected behavior?

  • Getting a word with a different length
  • or always being forced to handle possibly undefined values
  • or throwing/catching an exception?

EDIT: We could introduce two signatures for those methods though. One without length and a guaranteed string and one with length and string?.

@justinsmid
Copy link
Author

justinsmid commented Jul 5, 2022

I think i would personally expect it to be typed as string | undefined.

Getting a word with a different length when explicitly specifying a length seems like it'd be more troublesome, and having a function suddenly able to throw errors while other faker functions don't do so doesn't seem like a good idea either imo.

Splitting it up into separate methods like you suggested seems reasonable.

EDIT: Alternatively, we could use conditional types to return either string or string | undefined depending on whether a length was given. Something like this.

@xDivisionByZerox xDivisionByZerox added c: bug Something isn't working and removed s: pending triage Pending Triage labels Jul 5, 2022
@MasterOdin
Copy link

Could the function be made such that if you pass in a length that's greater than any word, it caps the value to the max word length? The few times I've used this function in the past, it was being passed into a function or whatever that doesn't allow undefined, and adding as string to every usage would be pretty annoying. Why would someone want this function to return undefined?

@ST-DDT
Copy link
Member

ST-DDT commented Jul 9, 2022

What about shorter words e.g. length = 1?

@MasterOdin
Copy link

length <= 0 also returns undefined for all locales, but I'd argue there to just return '' in those cases since. For length = 1, this only affects the hebrew locale, and there, maybe just bump it up to the next possible length (2)?

@hankucz
Copy link
Contributor

hankucz commented Aug 1, 2022

In #1218 I implemented it in a way that if a user expects too long word, the maximum possible length is given.

@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2022

Team decision

  • Throw an error by default, introduce options param that can be used to configure the errorHandling (e.g. return closest length).

Further discussions

  • Introduce min and max length range in a separate PR. word(number| {min: length, max: length, length, errorHandling})

@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2022

Note since we need this in multiple modules this should be done in a new central method: faker.helpers.stringArrayElement(elements, options)

@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2022

Note2 it should be applies/used for all of our word methods (those with a length parameters) in a single PR.

@Shinigami92
Copy link
Member

Just to raise my voice because I suffer in silence: I would suggest to name it strategy instead of errorHandling

@ST-DDT
Copy link
Member

ST-DDT commented Aug 4, 2022

OK.

strategy: 'fail' | 'return-any' = 'fail'

Or fail-if-unavailable? Or throw-if-unavailable?

@import-brain
Copy link
Member

OK.

strategy: 'fail' | 'return-any' = 'fail'

Or fail-if-unavailable? Or throw-if-unavailable?

What about just throw?

@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Sep 22, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Sep 22, 2022
@ST-DDT ST-DDT added the help wanted Extra attention is needed label Sep 22, 2022
@ST-DDT ST-DDT self-assigned this Sep 29, 2022
@ST-DDT ST-DDT moved this from Todo to Awaiting Review in Faker Roadmap Oct 2, 2022
Repository owner moved this from Awaiting Review to Done in Faker Roadmap Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working help wanted Extra attention is needed m: lorem Something is referring to the lorem module
Projects
No open projects
Status: Done
7 participants