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: export locales #642

Closed
wants to merge 1 commit into from
Closed

feat: export locales #642

wants to merge 1 commit into from

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Mar 21, 2022

Locales should be exported otherwise the only way to have localeFallback is to use the default exported Faker instance.

Example:

const en = require('@fakerjs/faker/locale/en');
const pt_BR = require('@fakerjs/faker/locale/pt_BR');
const { Faker } = require('@fakerjs/faker');

const faker = new Faker({ locales: {en, pt_BR}, locale: 'pt_BR', fallbackLocale: 'en' });

@mshima mshima requested a review from a team as a code owner March 21, 2022 13:58
@Shinigami92 Shinigami92 added c: feature Request for new feature needs test More tests are needed p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Mar 21, 2022
@Shinigami92
Copy link
Member

You need to change the bundling process somehow in

faker/scripts/bundle.ts

Lines 43 to 48 in 5642470

entryPoints: [
'./src/index.ts',
...Object.keys(locales).map((locale) => `./src/locale/${locale}.ts`),
'./src/iban.ts',
'./src/mersenne.ts',
],

Please also add tests to check each exported locale definition with an it.each

@Shinigami92 Shinigami92 marked this pull request as draft March 21, 2022 14:33
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #642 (0280c4b) into main (1b9a920) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main     #642    +/-   ##
========================================
  Coverage   99.67%   99.67%            
========================================
  Files        2119     2119            
  Lines      227439   227553   +114     
  Branches      982     1040    +58     
========================================
+ Hits       226691   226808   +117     
+ Misses        728      725     -3     
  Partials       20       20            
Impacted Files Coverage Δ
src/locale/af_ZA.ts 100.00% <100.00%> (ø)
src/locale/ar.ts 100.00% <100.00%> (ø)
src/locale/az.ts 100.00% <100.00%> (ø)
src/locale/cz.ts 100.00% <100.00%> (ø)
src/locale/de.ts 100.00% <100.00%> (ø)
src/locale/de_AT.ts 100.00% <100.00%> (ø)
src/locale/de_CH.ts 100.00% <100.00%> (ø)
src/locale/el.ts 100.00% <100.00%> (ø)
src/locale/en.ts 100.00% <100.00%> (ø)
src/locale/en_AU.ts 100.00% <100.00%> (ø)
... and 48 more

@Shinigami92 Shinigami92 changed the title Export locales. feat: export locales Mar 21, 2022
@Shinigami92 Shinigami92 removed the needs test More tests are needed label Mar 21, 2022
@mshima
Copy link
Contributor Author

mshima commented Mar 21, 2022

Thanks @Shinigami92 for the quick feedback.
Marking as ready for review, since it the requested changes have been addressed.

@mshima mshima marked this pull request as ready for review March 21, 2022 19:33
@Shinigami92
Copy link
Member

It's working? omg that would be really nice, thank you @mshima for this fix, I assume you will make many happy with that

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.

Currently we need to do something like this:

import { Faker } from "@faker-js/faker";
import es from "@faker-js/faker/locales/es";
import fr from "@faker-js/faker/locales/fr";

console.log(
  "Manually loaded locale:",
  new Faker({
    locales: { es: es.default, fr: fr.default },
    locale: "es",
    localeFallback: "fr",
  }).locales.es.title
); // Manually loaded locale: Spanish

This is working, but it would be much better if we can somehow make it possible to omit the need of using <locale>.default
Or do you know a better way of importing them?

But also there are no types defined for these exports 🤔
image
So at least this needs to be fixed before we can merge this PR!

Edit 1:
I found out that this <locale>.default is only an issue using esm, but not cjs
For cjs it's just:

import { Faker } from "@faker-js/faker";
import es from "@faker-js/faker/locales/es";
import fr from "@faker-js/faker/locales/fr";

console.log(
  "Manually loaded locale:",
  new Faker({
    locales: { es, fr },
    locale: "es",
    localeFallback: "fr",
  }).locales.es.title
); // Manually loaded locale: Spanish

Edit 2:
But only when you use TS with cjs 😆
When using cjs with raw js you need .default again 🤷

const { Faker } = require("@faker-js/faker");
const es = require("@faker-js/faker/locales/es");
const fr = require("@faker-js/faker/locales/fr");

console.log(
  "Manually loaded locale:",
  new Faker({
    locales: { es: es.default, fr: fr.default },
    locale: "es",
    localeFallback: "fr",
  }).locales.es.title
);

@mshima
Copy link
Contributor Author

mshima commented Mar 21, 2022

I found out that this <locale>.default is only an issue using esm, but not cjs

I think this is because of:

"node": "./dist/cjs/index.js",

This makes node esm to import cjs version.
Should be require instead of node.

@Shinigami92
Copy link
Member

I think this is because of:

"node": "./dist/cjs/index.js",

This makes node esm to import cjs version. Should be require instead of node.

I was not fully aware of what each of the things are doing when I crafted all together. Feel free to fix it until it's correct and I approved that everything is working.
I have a locale faker playground where I can now test esm TS/JS and cjs TS/JS.
I use pnpm's overrides to link to my local @faker-js/faker

@mshima
Copy link
Contributor Author

mshima commented Mar 21, 2022

I will checkout the project to see the generated code. Until now it could be done using CI 😄.

@mshima
Copy link
Contributor Author

mshima commented Mar 22, 2022

Currently we need to do something like this:

import { Faker } from "@faker-js/faker";
import es from "@faker-js/faker/locales/es";
import fr from "@faker-js/faker/locales/fr";

console.log(
  "Manually loaded locale:",
  new Faker({
    locales: { es: es.default, fr: fr.default },
    locale: "es",
    localeFallback: "fr",
  }).locales.es.title
); // Manually loaded locale: Spanish

This is working, but it would be much better if we can somehow make it possible to omit the need of using <locale>.default

Looks like this is a pattern esbuild uses.
See:

node -e 'console.log(require("@faker-js/faker"));'
{ Faker: [Getter], default: [Getter], faker: [Getter] }

Need to change the approach

@mshima
Copy link
Contributor Author

mshima commented Mar 22, 2022

Should work like:

const { Faker } = require("@faker-js/faker");
const { es, fr } = require("@faker-js/faker/locales");

console.log(
  "Manually loaded locale:",
  new Faker({
    locales: { es, fr },
    locale: "es",
    localeFallback: "fr",
  }).locales.es.title
);

src/locales/index.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

I will test this again this evening, due to I do not have the local playground on my workstation laptop but on my home pc

@Shinigami92
Copy link
Member

cjs js:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './locales/es' is not defined by "exports" in /home/shinigami/OpenSource/faker.js-tests/cjs/node_modules/@faker-js/faker/package.json

cjs ts:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './locales/es' is not defined by "exports" in /home/shinigami/OpenSource/faker.js-tests/cjs/node_modules/@faker-js/faker/package.json

esm js:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './locales/es' is not defined by "exports" in /home/shinigami/OpenSource/faker.js-tests/esm/node_modules/@faker-js/faker/package.json imported from /home/shinigami/OpenSource/faker.js-tests/esm/index.js

esm ts:

 [ERROR] Could not resolve "@faker-js/faker/locales/es"

    <stdin>:1:7:
      1  import "@faker-js/faker/locales/es"
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  The path "./locales/es" is not exported by package "@faker-js/faker":

    node_modules/@faker-js/faker/package.json:34:13:
      34    "exports": {              ^

  You can mark the path "@faker-js/faker/locales/es" as external to exclude it from the bundle,
  which will remove this error.

 [ERROR] Could not resolve "@faker-js/faker/locales/fr"

    <stdin>:1:7:
      1  import "@faker-js/faker/locales/fr"
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  The path "./locales/fr" is not exported by package "@faker-js/faker":

    node_modules/@faker-js/faker/package.json:34:13:
      34    "exports": {              ^

  You can mark the path "@faker-js/faker/locales/fr" as external to exclude it from the bundle,
  which will remove this error.

/home/shinigami/OpenSource/faker.js-tests/esm/node_modules/.pnpm/esbuild@0.14.12/node_modules/esbuild/lib/main.js:1557
  let error = new Error(`${text}${summary}`);
              ^

Error: Build failed with 1 error:
<stdin>:1:7: ERROR: Could not resolve "@faker-js/faker/locales/es"

Types also doesn't resolve

I will setup a faker playground repo that you can checkout to test it also yourself

@Shinigami92
Copy link
Member

@mshima I created a playground here: https://github.com/faker-js/playground
Feel free to ask if something is unclear

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Apr 19, 2022
@mshima mshima requested a review from Shinigami92 May 30, 2022 20:36
@Shinigami92
Copy link
Member

@mshima I see this PR and I like it, but currently our maintainer resources are a bit limited and we have a bunch of other things on our agenda. So it still could take a bit until we can proceed with this wanted feature.
We want to internally discuss if e.g. we want imports like

  • import { locale } from '@faker-js/faker/locale/de'
  • import { de } from '@faker-js/faker/locale/de'
  • import { de } from '@faker-js/faker/locales'
  • import { de } from '@faker-js/faker'
    And for the long-term (not this PR) we even think about using a mono-repo structure and make possible to use
import { de } from '@faker-js/de'
import { Faker } from '@faker-js/faker'

new Faker({ /* ... use de somehow */ })

@mshima
Copy link
Contributor Author

mshima commented Jun 1, 2022

@Shinigami92 no problem, I cannot use v7 for now since we need node 12 support.

@mshima
Copy link
Contributor Author

mshima commented Jun 1, 2022

I think these makes more sense

  • import { de } from '@faker-js/faker/locales'
  • import { de } from '@faker-js/faker'

@mshima
Copy link
Contributor Author

mshima commented Sep 5, 2022

@mshima I see this PR and I like it, but currently our maintainer resources are a bit limited and we have a bunch of other things on our agenda. So it still could take a bit until we can proceed with this wanted feature. We want to internally discuss if e.g. we want imports like

  • import { locale } from '@faker-js/faker/locale/de'
  • import { de } from '@faker-js/faker/locale/de'
  • import { de } from '@faker-js/faker/locales'
  • import { de } from '@faker-js/faker'
    And for the long-term (not this PR) we even think about using a mono-repo structure and make possible to use
import { de } from '@faker-js/de'
import { Faker } from '@faker-js/faker'

new Faker({ /* ... use de somehow */ })

@Shinigami92 is there a decision about the api? We are about to start a major version, so I may work on this soon.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 5, 2022

We will discuss this in the next team meeting again.

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision and removed s: accepted Accepted feature / Confirmed bug labels Sep 5, 2022
@ST-DDT ST-DDT added s: on hold Blocked by something or frozen to avoid conflicts and removed s: needs decision Needs team/maintainer decision needs rebase There is a merge conflict labels Sep 8, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2022

@Shinigami92 will tackle this in a new PR for v8.
We would like to keep this open for now, to make the discussion that happened here more visible.

@mshima
Copy link
Contributor Author

mshima commented Oct 15, 2022

As workaround:

  let nativeFakerInstance;
  // Faker >=6 doesn't exports locale by itself, it only exports a faker instance with the locale.
  // We need a Faker instance for each entity, to build additional fake instances, use the locale from the exported localized faker instance.
  // See https://github.com/faker-js/faker/pull/642
  try {
    // eslint-disable-next-line import/no-dynamic-require
    nativeFakerInstance = (await import(`@faker-js/faker/locale/${nativeLanguage}`)).faker;
  } catch (error) {
    // Faker not implemented for the native language, fallback to en.
    // eslint-disable-next-line import/no-unresolved, import/no-dynamic-require
    nativeFakerInstance = (await import('@faker-js/faker/locale/en')).faker;
  }

  const faker = new Faker({
    locales: nativeFakerInstance.locales,
    locale: nativeFakerInstance.locale,
    localeFallback: nativeFakerInstance.localeFallback,
  });

@import-brain import-brain added the has workaround Workaround provided or linked label Oct 15, 2022
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Nov 21, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 20, 2023

The locales can now be imported via import { de } from '@faker-js/faker' and import { fakerDE as faker } from '@faker-js/faker'.

Superseded by #1735

@ST-DDT ST-DDT closed this Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature has workaround Workaround provided or linked needs rebase There is a merge conflict p: 1-normal Nothing urgent s: on hold Blocked by something or frozen to avoid conflicts
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants