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: split SimpleFaker class from Faker #2369

Merged
merged 26 commits into from Sep 18, 2023

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 added this to the v8.1 - Split Faker Class milestone Sep 1, 2023
@Shinigami92 Shinigami92 self-assigned this Sep 1, 2023
@Shinigami92
Copy link
Member Author

In DateModule we have a reference to definitions

https://github.com/faker-js/faker/pull/2369/files#diff-33d9552c7d1fb7f98cb50e15888b577d2609cd9b6f17a333b641752c5049629dR1001

I have multiple ideas how to solve this:

  • Split a BaseDateModule from DateModule (theoretically we could do that with other modules as well)
  • Add a check whether we are currently have definitions or not, if not the throw Error and indicate to use Faker instance instead
  • open for alternative ideas

@xDivisionByZerox
Copy link
Member

In DateModule we have a reference to definitions

#2369 (files)

I have multiple ideas how to solve this:

  • Split a BaseDateModule from DateModule (theoretically we could do that with other modules as well)
  • Add a check whether we are currently have definitions or not, if not the throw Error and indicate to use Faker instance instead
  • open for alternative ideas

IMO we should simply exclude the date module from the BaseFaker class. Gernerally, if a module uses locale data, exclude it. If you want a Date you can still do new Date(faker.number.int({ ... })). I would take the easy route here.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 1, 2023

IMO Dates are very useful for everyday testing.

@Shinigami92
Copy link
Member Author

Shinigami92 commented Sep 1, 2023

Originally I just put it there because datatype uses date.between
But datatype is deprecated 👀


Well... datatype has boolean, that is needed for helpers... helpers has multiple and other nice to have stuff
it's like a waterfall, and all of that just because of month names in date module

@Shinigami92
Copy link
Member Author

Okay... I tested building a BaseDateModule, but this is bad, because it produces faker.date conflicts between the BaseFaker and Faker class...

@Shinigami92 Shinigami92 marked this pull request as ready for review September 1, 2023 20:37
@Shinigami92 Shinigami92 requested a review from a team as a code owner September 1, 2023 20:37
@Shinigami92 Shinigami92 changed the title Split BaseFaker class from Faker feat: split BaseFaker class from Faker Sep 1, 2023
@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #2369 (0ace8eb) into next (f40e217) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2369      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files        2800     2801       +1     
  Lines      252410   252469      +59     
  Branches     1100     1103       +3     
==========================================
+ Hits       251435   251482      +47     
- Misses        948      960      +12     
  Partials       27       27              
Files Changed Coverage Δ
src/faker.ts 88.23% <100.00%> (-3.83%) ⬇️
src/index.ts 100.00% <100.00%> (ø)
src/internal/bind-this-to-member-functions.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 100.00% <100.00%> (ø)
src/modules/date/index.ts 100.00% <100.00%> (ø)
src/modules/helpers/index.ts 99.12% <100.00%> (+<0.01%) ⬆️
src/modules/number/index.ts 100.00% <100.00%> (ø)
src/modules/string/index.ts 100.00% <100.00%> (ø)
src/simple-faker.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@Shinigami92
Copy link
Member Author

Potentially TODOs:

  • add docs about baseFaker/BaseFaker
  • add tests?!
  • discuss if we want a @faker-js/faker/base
  • export type Base*Module

@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Sep 1, 2023
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent do NOT merge yet Do not merge this PR into the target branch yet labels Sep 1, 2023
@ST-DDT ST-DDT linked an issue Sep 7, 2023 that may be closed by this pull request
@ST-DDT
Copy link
Member

ST-DDT commented Sep 7, 2023

We are not sure about what the actual questions here are.

Team Decision

  • add docs about baseFaker/BaseFaker

Yes

  • add tests?!

Only tests that concern creating the base faker instance.

  • discuss if we want a @faker-js/faker/base

Currently, we don't see a need for that, we should rather fix the tree-shaking.
Because if we add that now and remove it in v9 this will result in bad DX.

  • export type Base*Module

Yes, only as types.

  • Additionally
  • Split BaseFaker+baseFaker into it's own file

@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Sep 7, 2023
@Shinigami92 Shinigami92 marked this pull request as draft September 7, 2023 16:28
@Shinigami92 Shinigami92 force-pushed the 1351-split-faker-class-into-smaller-unit branch from 9d6d241 to e2b654c Compare September 7, 2023 16:28
scripts/apidoc/baseFakerClass.ts Outdated Show resolved Hide resolved
src/base-faker.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 marked this pull request as ready for review September 7, 2023 17:07
@Shinigami92 Shinigami92 removed the do NOT merge yet Do not merge this PR into the target branch yet label Sep 7, 2023
scripts/apidoc/baseFakerClass.ts Outdated Show resolved Hide resolved
src/base-faker.ts Outdated Show resolved Hide resolved
scripts/apidoc/baseFakerClass.ts Outdated Show resolved Hide resolved
src/base-faker.ts Outdated Show resolved Hide resolved
src/base-faker.ts Outdated Show resolved Hide resolved
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.

It would be good to document what the difference is between fakerBASE and BaseFaker in /guide/localization.html since they sound confusingly similar.

@Shinigami92 Shinigami92 marked this pull request as draft September 13, 2023 18:10
@Shinigami92 Shinigami92 marked this pull request as ready for review September 13, 2023 19:08
ST-DDT
ST-DDT previously approved these changes Sep 13, 2023
@ST-DDT ST-DDT requested review from a team September 13, 2023 22:58
Co-authored-by: xDivisionByZerox <leyla.jaehnig@gmx.de>
src/simple-faker.ts Outdated Show resolved Hide resolved
docs/guide/usage.md Show resolved Hide resolved
docs/guide/usage.md Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested a review from a team September 15, 2023 20:16
@Shinigami92 Shinigami92 enabled auto-merge (squash) September 18, 2023 03:52
@Shinigami92 Shinigami92 merged commit d6a4f8c into next Sep 18, 2023
19 checks passed
@Shinigami92 Shinigami92 deleted the 1351-split-faker-class-into-smaller-unit branch September 18, 2023 03:53
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 p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split the Faker class into smaller units
4 participants