-
-
Notifications
You must be signed in to change notification settings - Fork 931
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.97% 99.97% -0.01%
==========================================
Files 2798 2800 +2
Lines 227375 227385 +10
Branches 956 580 -376
==========================================
+ Hits 227310 227319 +9
- Misses 65 66 +1
|
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.
LGTM
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.
see my comment at #2667 (comment) - would it make sense to delay merging this?
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.
changed my feedback to a comment to avoid a hard block, but i think there is still some logic in deferring this and merging together with the actual changes.
e659674
First part of the standalone module function feature #2667
Overview
Description
Create the fakerCore container object.