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

Custom faker instance with en locale cannot generate system variables #3405

Closed
9 of 10 tasks
terrymun opened this issue Feb 18, 2025 · 10 comments · May be fixed by #3406
Closed
9 of 10 tasks

Custom faker instance with en locale cannot generate system variables #3405

terrymun opened this issue Feb 18, 2025 · 10 comments · May be fixed by #3406
Assignees
Labels
c: docs Improvements or additions to documentation c: locale Permutes locale definitions p: 1-normal Nothing urgent question Further information is requested
Milestone

Comments

@terrymun
Copy link

Pre-Checks

Describe the bug

I am using a custom faker instance in my app, i.e.:

import { faker, Faker, en } from '@faker-js/faker';

const customFaker = new Faker({
  locale: en,
  seed: 1337,
});
console.log(customFaker.system.fileName());

However, I get an error in the console with this message:

"The locale data for 'system.mime_type' are missing in this locale"

However, en is the locale with the widest / largest dataset and I would expect it to have system.mine_type implemented. Using the default faker instance does not give me any issues, but I assume that the default faker is also using the "en" locale.

Minimal reproduction code

See example here: https://stackblitz.com/edit/vitejs-vite-iwf3husr

To reproduce this code independently, you can do this:

import { faker, Faker, en } from '@faker-js/faker';

const customFaker = new Faker({
  locale: en,
  seed: 1337,
});

// NOTE: This works!
console.log(faker.system.fileName());

// NOTE: This will throw an error in the console
//       "The locale data for 'system.mime_type' are missing in this locale"
console.log(customFaker.system.fileName());

Additional Context

No response

Environment Info

System:
  OS: Linux 5.0 undefined
  CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Memory: 0 Bytes / 0 Bytes
  Shell: 1.0 - /bin/jsh
Binaries:
  Node: 18.20.3 - /usr/local/bin/node
  Yarn: 1.22.19 - /usr/local/bin/yarn
  npm: 10.2.3 - /usr/local/bin/npm
  pnpm: 8.15.6 - /usr/local/bin/pnpm
npmPackages:
  @faker-js/faker: ^9.5.0 => 9.5.0

Which module system do you use?

  • CJS
  • ESM

Used Package Manager

npm

@terrymun terrymun added c: bug Something isn't working s: pending triage Pending Triage labels Feb 18, 2025
@terrymun
Copy link
Author

Realized that I need to extend locale with base as well, my bad.

@ST-DDT ST-DDT added question Further information is requested p: 1-normal Nothing urgent c: locale Permutes locale definitions and removed c: bug Something isn't working s: pending triage Pending Triage labels Feb 18, 2025
@matthewmayer
Copy link
Contributor

matthewmayer commented Feb 19, 2025

I wonder if there is a way we could emit a warning if someone makes a custom faker without extending base? As I feel this is likely to be not intentional most of the time.

@matthewmayer

This comment has been minimized.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2025

Maybe we should add a hint to the constructor like

/**
 * ...
 * Please make sure that all required locales including parent locales are present e.g. `[de_AT, de, en, base]`
 * ...
 */

And extend the error message a bit:

    throw new FakerError(
      `The locale data for '${path.join('.')}' are missing in this locale.
  If this is a custom Faker instance, please make sure all required locales are used e.g. '[de_AT, de, en, base]'.
  Please contribute missing data to the project or use a locale/Faker instance that has these data.
  For more information see https://fakerjs.dev/guide/localization.html`
    );

?

@ST-DDT ST-DDT added the c: docs Improvements or additions to documentation label Feb 19, 2025
@terrymun
Copy link
Author

@ST-DDT That's a great idea. I have to admit though the error I encountered is a user error coming from myself because I totally missed the part of extending off base in the docs...

@matthewmayer
Copy link
Contributor

Still we can always improve error messages etc to reduce human error 😀

By the way why did you feel you had to make a custom faker instance - if you just need a custom seed you don't need one? Or was this a simplified example just to explain the bug and you actually need a custom Faker instance?

@terrymun
Copy link
Author

@matthewmayer Your second guess is correct: we needed to create a custom faker instance with additional fake data, and the example I shown is simply a contrived one but minimal enough to reproduce the "issue" (which turns out to be a user error, hehe)

@matthewmayer
Copy link
Contributor

matthewmayer commented Feb 19, 2025

@ST-DDT what do you think about adding a check like this to the constructor?

    if (locale && (typeof locale === 'string' || locale.length === 1)) {
      // eslint-disable-next-line no-undef
      console.warn(
        '[@faker-js/faker] only a single locale was provided. Was this intentional? In general, pass an array of locales ending with the `base` locale, for example [customLocale, de_CH, de, en, base]. See https://fakerjs.dev/guide/localization.html#custom-locales-and-fallbacks'
      );
    }

@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2025

If you really want to save some bundle size, you might want to create a locale instance that only contains the data you need prepackaged.
So IMO the single locale usecase is too useful to interfere with it using a warning.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 19, 2025

I created PR to improve the error message: #3406

@ST-DDT ST-DDT self-assigned this Feb 19, 2025
@ST-DDT ST-DDT added this to the vAnytime milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation c: locale Permutes locale definitions p: 1-normal Nothing urgent question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants