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

Remove locale switching support #1340

Closed
ST-DDT opened this issue Sep 6, 2022 · 16 comments · Fixed by #1735
Closed

Remove locale switching support #1340

ST-DDT opened this issue Sep 6, 2022 · 16 comments · Fixed by #1735
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: feature Request for new feature c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Sep 6, 2022

Clear and concise description of the problem

The locale switching support adds a lot of overhead to the faker instance.
Especially the default instance is huge.

Suggested solution

  • Remove locale switching support from Faker.
  • The locales have to be set at creation time new Faker([ en ])
  • It is possible to provide a single instance as well as a list of locales, that will be merged immediately
  • The locale merging behavior isn't changed to the version as it is now, but is exported for users to use themselves (e.g. in build scripts)
  • The locale data can be accessed from both cjs and esm using simple imports (Maybe covered by feat: export locales #642)
  • The main faker instance is only en
  • The documentation contains a hint regarding the use of other locales (Maybe handled as part of Document how to use locales #1322)
  • The provided faker instances use a proper locale hierarchy fr_CH -> fr -> en
  • Global data such as smiley/iban/mime-types will be moved to their own locale (TBD: global or individual)

Alternative

No response

Additional context

No response

@ST-DDT ST-DDT added c: feature Request for new feature s: on hold Blocked by something or frozen to avoid conflicts p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release labels Sep 6, 2022
@ST-DDT ST-DDT added this to the v8.0 - Module Re-Shuffling milestone Sep 6, 2022
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Sep 6, 2022
@xDivisionByZerox
Copy link
Member

Global data such as smiley/iban/mime-types will be moved to their own locale (TBD: global or individual)

Why cant they be scoped to the module?

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 6, 2022

They could be. If we drop support for replacing them.
However, that would be another breaking change that should be it's own ticket for better visibility.
Do we have that already somewhere? I remember discussing this before.

@Shinigami92
Copy link
Member

I'm somewhat interested to be champion of this, but have some other issues I already provide higher prio from my work-amount
So if someone else claims this before I can start, I'm also fine with that

@matthewmayer
Copy link
Contributor

matthewmayer commented Feb 3, 2023

Just to clarify, if you're currently using a single locale like this:

import { faker } from '@faker-js/faker';
// or with CJS, 
// const {faker} = require("@faker-js/faker")
faker.locale = "de" //or setLocale
console.log(faker.word.noun()) //Glücksspiel

The recommended migration will be to switch to

import { faker } from '@faker-js/faker/locale/de';
// or with CJS, 
// const {faker} = require("@faker-js/faker/locale/de")
console.log(faker.word.noun()) //Glücksspiel

?

@ST-DDT ST-DDT self-assigned this Feb 3, 2023
@Shinigami92
Copy link
Member

Shinigami92 commented Feb 3, 2023

We are not sure yet if we still want to support import { faker } from '@faker-js/faker/locale/de' in the long run, but yes.

Our actual plan is to encourage using one of these options (might be slightly different in real world after we done it):

import { faker, fakerDE, localeDE, ... } from '@faker-js/faker'

fakerDE.word.noun()

const myFaker = new Faker({ locale: [localeDE, ...] })
myFaker.word.noun()

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 3, 2023

Yes or

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

If we use #1735.

@Shinigami92
Copy link
Member

Looks like we exactly woke up the same time 🤣

@ST-DDT ST-DDT moved this from Todo to In Progress in Faker Roadmap Feb 3, 2023
@matthewmayer
Copy link
Contributor

Would it make sense to land this in v9, and in v8 switching locales to a non-en locale still works but add a smaller PR which shows a deprecation message encouraging you to switch to the locale-specific import syntax now?

@Shinigami92
Copy link
Member

Would it make sense to land this in v9, and in v8 switching locales to a non-en locale still works but add a smaller PR which shows a deprecation message encouraging you to switch to the locale-specific import syntax now?

We currently didn't find a way how we could achieve that
Do you have any idea?

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 3, 2023

We could support this:

faker.locale = localeDE;

but not the currently supported:

faker.locale = 'de';

@Shinigami92
Copy link
Member

We could support this:

faker.locale = localeDE;

but not the currently supported:

faker.locale = 'de';

Yes, but this wont help as it would be a total breaking change anyways and also does not lead into the direction we are aiming for anyways.
So to me that does not make much sense.

@matthewmayer
Copy link
Contributor

Would it make sense to land this in v9, and in v8 switching locales to a non-en locale still works but add a smaller PR which shows a deprecation message encouraging you to switch to the locale-specific import syntax now?

We currently didn't find a way how we could achieve that Do you have any idea?

rough idea something like #1812

@matthewmayer
Copy link
Contributor

Yes or

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

If we use #1735.

One thing i don't really like about the

import { fakerDE } from '@faker-js/faker'; syntax is that if i change my mind about which locale to support, i now have to change my code in lots of places.

e.g. if i was previously using

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

and then i have lots of calls in my code like

faker.person.name()
faker.location.address()
faker.internet.email()

and then i read the docs and realize i can get more realistic data for my use-case by using the en-GB locale, previously i had to add one line, but now I have to do

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

and then i have to change lots of other lines of code like

fakerEN_GB.person.name()
fakerEN_GB.location.address()
fakerEN_GB.internet.email()

and anywhere where the docs refer to faker.foo.bar i have to remember to do fakerEN_GB.foo.bar
should the docs perhaps encourage instead

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

?

@Shinigami92
Copy link
Member

should the docs perhaps encourage instead

import { fakerDE as faker } from '@faker-js/faker';
?

IMO yes, they should 👍
Or at least with a hint, to do this if preferred

@matthewmayer
Copy link
Contributor

matthewmayer commented Feb 18, 2023

i think the main README and https://fakerjs.dev/guide/usage.html should probably show the most common use-cases for importing only ie

  1. Default/English only (ESM and CJS)
    e.g. import { faker } from '@faker-js/faker';

  2. Other language with default fallback (ESM and CJS)
    e.g. import { fakerDE as faker } from '@faker-js/faker';

And then more unusual cases could be linked to
https://fakerjs.dev/guide/localization.html e.g.

  • how to avoid loading the full bundle
  • how to customize the fallback locales
  • how to import multiple locales

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 19, 2023

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

Done

And then more unusual cases could be linked to
fakerjs.dev/guide/localization.html

Done, not sure if you had that in mind: 858020c (#1735)

@github-project-automation github-project-automation bot moved this from In Progress to Done in Faker Roadmap Mar 7, 2023
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 c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants