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

fix: force passed locales into faker constructor #580

Merged
merged 10 commits into from
Mar 28, 2022

Conversation

vith
Copy link
Contributor

@vith vith commented Feb 28, 2022

I saw that there's a Faker class being exported now, so I tried to use it, but it doesn't seem to work.

For example, when trying to do faker = new Faker(); faker.name.firstName() I get:

TypeError: Cannot read properties of undefined (reading 'name')

I didn't see any examples of using this constructor in the docs, so it's possible I'm using it wrong. Anyway, I wrote these tests to check if it was a problem in my own project, but they fail as well in the same way.

@vith vith requested a review from a team as a code owner February 28, 2022 12:09
@Shinigami92
Copy link
Member

I think it fails, because you don't provide any locales.

Maybe we need to change the type on your side to force to provide a locales 🤔

@Shinigami92
Copy link
Member

Uhm... tests are green? 👀

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #580 (fed2179) into main (814880b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head fed2179 differs from pull request most recent head 1870549. Consider uploading reports for the commit 1870549 to get more accurate results

@@           Coverage Diff           @@
##             main     #580   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1923     1923           
  Lines      176891   176903   +12     
  Branches      896      901    +5     
=======================================
+ Hits       175735   175748   +13     
+ Misses       1100     1099    -1     
  Partials       56       56           
Impacted Files Coverage Δ
src/faker.ts 100.00% <100.00%> (ø)
src/index.ts 97.43% <0.00%> (+2.56%) ⬆️

@ST-DDT
Copy link
Member

ST-DDT commented Feb 28, 2022

I assume since it probably returns undefined in the middle the fallback of ['a', 'b', 'c'] applies and thus passes this trivial test.
Maybe change it to length > 1 (as there shouldn't be any one char names in our data).

See also: #581

@vith
Copy link
Contributor Author

vith commented Feb 28, 2022

The tests are green just because I defined them with it.fails. If you want I can remove that so they actually cause the suite to fail now.

I see what you mean now about not providing any locales, i.e. I need replicate something like this:

import { Faker } from '../faker';
import sv from '../locales/sv';
import en from '../locales/en';

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

export = faker;

But I haven't found any way to actually import the locale data externally. I think the exports field of package.json only lets me import the pre-instantiated per-locale singletons like the one I pasted above from src/locale/sv.ts?

@ST-DDT
Copy link
Member

ST-DDT commented Feb 28, 2022

The tests are green just because I defined them with it.fails. If you want I can remove that so they actually cause the suite to fail now.

Yes, that would make it more obvious for anyone looking at the tests.

But I haven't found any way to actually import the locale data externally. I think the exports field of package.json only lets me import the pre-instantiated per-locale singletons like the one I pasted above from src/locale/sv.ts?

Yes, currently there isn't because it wasn't in 5.x. This is planned for a release after 6.0 when we actually start adding fixes and features.

@vith
Copy link
Contributor Author

vith commented Mar 1, 2022

Yes, that would make it more obvious for anyone looking at the tests.

Done.

Yes, currently there isn't because it wasn't in 5.x. This is planned for a release after 6.0 when we actually start adding fixes and features.

Ah, okay. I wonder if the Faker class shouldn't be exported from the package in 6.0 then, if the locale data it needs won't be?

I tried to use the class because I got it as a code completion via the TypeScript Language Server in VSCode.

Not sure if there's anything useful to make out of the tests in this PR then. I guess making the locales field required in the constructor would help like @Shinigami92 said, and/or a better error message when locale data is missing.

@ST-DDT
Copy link
Member

ST-DDT commented Mar 1, 2022

Could you adjust the constructor to not have a default and require the locales?

@Shinigami92
Copy link
Member

Could you adjust the constructor to not have a default and require the locales?

@vith Would you like to open a new PR to do this or maybe just use this PR?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 10, 2022

Could you adjust the constructor to not have a default and require the locales?

@vith Would you like to open a new PR to do this or maybe just use this PR?

I recommend using this one, as both parts fix+test belong together anyway.

@Shinigami92 Shinigami92 added the p: 1-normal Nothing urgent label Mar 15, 2022
@Shinigami92
Copy link
Member

I will jump into this PR now and fix the constructor

@Shinigami92 Shinigami92 changed the title test(constructor): add failing tests for new Faker use fix: force passed locales into faker constructor Mar 21, 2022
@Shinigami92 Shinigami92 self-assigned this Mar 21, 2022
@Shinigami92 Shinigami92 marked this pull request as draft March 21, 2022 19:35
@Shinigami92 Shinigami92 marked this pull request as ready for review March 21, 2022 19:50
src/faker.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested review from ST-DDT and a team March 21, 2022 20:08
ST-DDT
ST-DDT previously approved these changes Mar 21, 2022
@Shinigami92 Shinigami92 requested a review from a team March 21, 2022 20:38
@Shinigami92 Shinigami92 added the s: accepted Accepted feature / Confirmed bug label Mar 21, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 24, 2022

This needs to be rebased

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Mar 24, 2022
@ST-DDT ST-DDT requested a review from a team March 24, 2022 10:39
@Shinigami92 Shinigami92 requested a review from a team March 25, 2022 18:42
@Shinigami92 Shinigami92 enabled auto-merge (squash) March 28, 2022 16:49
@Shinigami92 Shinigami92 merged commit 5ed963f into faker-js:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: test p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants