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

feat: introduce locale proxy #2004

Merged
merged 46 commits into from
Apr 23, 2023
Merged

feat: introduce locale proxy #2004

merged 46 commits into from
Apr 23, 2023

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 1, 2023

Adds #893

Changes

  • Converts faker.definitions to a proxy that throws if faker.definitions.category?.entry is missing.
  • Adds faker.rawDefinitions to access optional data

Types

  • faker.definitions.person.first_name[0]: string
  • faker.rawDefinitions.person?.first_name?.[0]: string | undefined

Usage

  • faker.helpers.arrayElement(faker.definitions.person.first_name)
  • faker.helpers.arrayElement(faker.rawDefinitions.person?.first_name ?? ['alternatives'])

@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug breaking change Cannot be merged when next version is not a major release labels Apr 1, 2023
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Apr 1, 2023
@ST-DDT ST-DDT self-assigned this Apr 1, 2023
@ST-DDT ST-DDT requested a review from a team as a code owner April 1, 2023 22:07
@ST-DDT ST-DDT requested a review from a team April 1, 2023 22:07
@codecov
Copy link

codecov bot commented Apr 1, 2023

Codecov Report

Merging #2004 (f915d49) into next (2675ec2) will decrease coverage by 0.01%.
The diff coverage is 99.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2004      +/-   ##
==========================================
- Coverage   99.59%   99.59%   -0.01%     
==========================================
  Files        2540     2541       +1     
  Lines      242706   242800      +94     
  Branches     1299     1317      +18     
==========================================
+ Hits       241723   241815      +92     
- Misses        956      958       +2     
  Partials       27       27              
Impacted Files Coverage Δ
src/definitions/definitions.ts 0.00% <0.00%> (ø)
src/faker.ts 91.86% <100.00%> (+0.06%) ⬆️
src/locale-proxy.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 99.06% <100.00%> (ø)
src/modules/location/index.ts 99.17% <100.00%> (+0.16%) ⬆️
src/modules/person/index.ts 98.04% <100.00%> (ø)

... and 1 file with indirect coverage changes

src/locale-proxy.ts Outdated Show resolved Hide resolved
src/locale-proxy.ts Outdated Show resolved Hide resolved
src/locale-proxy.ts Outdated Show resolved Hide resolved
src/locale-proxy.ts Outdated Show resolved Hide resolved
src/locale-proxy.ts Outdated Show resolved Hide resolved
src/locale-proxy.ts Outdated Show resolved Hide resolved
test/all_functional.spec.ts Outdated Show resolved Hide resolved
test/all_functional.spec.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 1, 2023

Soft blocked by #2005

test/faker.spec.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

Suppose we decide some definitions are not wanted for en for example we remove https://github.com/faker-js/faker/blob/next/src/locales/en/location/county.ts and it would only be available in selected locales like en_US.

What would it be replaced by? Completely remove the file? Or

export default [];

Do we intend there for there to be a semantic difference between a missing definition and an empty array?

@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 2, 2023

What would it be replaced by? Completely remove the file? Or

export default [];

I think the [] is normally a good solution + a comment that the user should use a country specific variant instead.
But that would cause issues for all other locales 🤔, so maybe some dummy counties (en_US ones as they end in County)?

Do we intend there for there to be a semantic difference between a missing definition and an empty array?

Yes. nullish => Missing. [] => not applicable

if (value == null) {
throw new FakerError(
`The locale data for '${categoryName}.${entryName.toString()}' are missing in this locale.
Please contribute the missing data to the project or use a locale/Faker instance that has these data.
For more information see https://next.fakerjs.dev/guide/localization.html`
);
} else if (Array.isArray(value) && value.length === 0) {
throw new FakerError(
`The locale data for '${categoryName}.${entryName.toString()}' aren't applicable to this locale.
If you think this is a bug, please report it at: https://github.com/faker-js/faker`
);

Copy link
Contributor

@matthewmayer matthewmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document this in migration guide as a breaking change, something like this:


Attempting to access a method where Faker has no data will now throw a FakerError instead of returning undefined. Since the majority of methods have data in at least one fallback language (e.g. English), this error is likely to only happen in two situations

  1. Accessing data which is not applicable for the locale, for example Chinese does not use prefixes in names:
import { faker, fakerZH_CN } from '@faker-js/faker';

faker.name.prefix() // 'Mr'
fakerZH_CN.person.prefix() // throws a FakerError
  1. When using a custom Faker instance rather than one of the prebuilt instances.
import { Faker, fakerES, es } from '@faker-js/faker';

const fakerES_nofallbacks = new Faker({
  locale: [es],
});
fakerES.music.songName() // 'I Want to Hold Your Hand' (fallback from en)
fakerES_nofallbacks.music.songName() // throws a FakerError

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there should be merged some other PRs before this to reduce the diff
Maybe mark it as draft or label on hold or whatever, so reviewers are notified again when this PR is ready again

test/locale-proxy.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from a team April 2, 2023 12:19
@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Apr 21, 2023
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 21, 2023

Blocked by #2078

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Apr 21, 2023
@ST-DDT ST-DDT removed the s: on hold Blocked by something or frozen to avoid conflicts label Apr 23, 2023
src/locale-proxy.ts Outdated Show resolved Hide resolved
test/all_functional.spec.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member Author

ST-DDT commented Apr 23, 2023

Ready to review.

@ST-DDT ST-DDT requested review from xDivisionByZerox and a team April 23, 2023 11:28
@ST-DDT ST-DDT requested review from Shinigami92 and a team April 23, 2023 12:18
@ST-DDT ST-DDT changed the title feat: introduce locale access proxy feat: introduce locale proxy Apr 23, 2023
@ST-DDT ST-DDT merged commit 8a0bbf5 into next Apr 23, 2023
17 checks passed
@ST-DDT ST-DDT deleted the feat/locale-access-proxy branch April 23, 2023 16:51
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: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Introduce a way to get details about missing locale data when using them
4 participants