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: introduce FakerCore #2838
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the PR is still in draft but I would like to provide some early-feedback
- I think the
refDate
should still be named something likedefault
orfallback
orbase
, so the intent is more clear and there might be no variable naming conflict when using an option's refDate + the ...RefDate - I suggest to not do something like
super({ fakerCore: fakerCore ?? { locale, randomizer, config: {} } })
but only pass the locale and randomizer so the config get's set like every other method call to a default internally. Or is there something special about that I currently overlook? 👀
|
Does anybody know why this test is failing with that particular error message?
It seems to happen only with vitest/CJS. |
If I set Or it is related to |
The error is caused by this part: _class.prototype.__init23.call(this);,this.definitions=_chunkWAQHUEHTcjs.d.call(void 0, this.rawDefinitions)
^ Which is equivalent to this source code: Lines 195 to 197 in e4da2f5
|
I created an issue in tsup with a minimal reproduction: And it has reeeaaally specific conditions, check the issue for more details. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2838 +/- ##
==========================================
- Coverage 99.96% 99.95% -0.01%
==========================================
Files 2977 2977
Lines 215422 215463 +41
Branches 950 954 +4
==========================================
+ Hits 215351 215375 +24
- Misses 71 88 +17
|
get rawDefinitions(): LocaleDefinition { | ||
// TODO @ST-DDT 2024-05-14: Should we deprecate this? | ||
return this.fakerCore.locale; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecate this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give reasoning on why we should be doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you could access them via fakerCore directly.
Later: I thought about potentially removing faker.definitions in favor of an explicit method call as well.
faker.definitions.module.entry
=> requireData(fakerCore, 'module', 'entry')
EDIT: requireData()
is required for the standalone module functions rewrite because there is no longer a faker.definitions
to call to fail on missing data.
First part of the standalone module function feature #2667