Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
How should we distinguish between module functions and helper code for modules? e.g. https://github.com/faker-js/faker/blob/next/src/modules/finance/bitcoin.ts I considered prefixing the helper files with So that:
As an alternative, we could add an What do you think? |
63d3440 to
b1c0f00
Compare
|
Which variant do you prefer? A) resolveLocaleData(fakerCore, 'commerce', 'product_name')or B) assertLocaleData(fakerCore.definitions.commerce?.product_name, 'commerce.product_name')A is shorter, but you can click on B to navigate to the source definitions. Also auto completion works better with B. |
I need some context here:
Also I have the strong feeling that the standalone module function PRs would get more traction from the core team if they could be done in smaller chunks. Like maybe start with one module and let everyone experience of it works. Since modules have to stay anyway, there would be no harm in simply adding one module at the time. We could even release them as "developer preview" to gather intel on how they "feel" in every day coding. Also, I'm aware that the "transform-once" script exists, but:
I'm not saying it doesn't work! Throwing it at the entire code base at once might be a bit overwhelming I'd say. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #3748 +/- ##
==========================================
+ Coverage 98.88% 99.03% +0.14%
==========================================
Files 886 1182 +296
Lines 3062 3724 +662
Branches 556 672 +116
==========================================
+ Hits 3028 3688 +660
- Misses 30 31 +1
- Partials 4 5 +1
🚀 New features to boost your workflow:
|
faker/src/internal/locale-proxy.ts Lines 57 to 63 in 626865f faker/src/utils/resolve-locale-data.ts Lines 18 to 37 in 43f5d71 It needs to the string to include it in the actual error message.
Theoretically yes, but that's not what they are for. The longer I think about this, the more I prefer B.
It was never meant to do all the work.
This is kind of chicken and egg problem. Sounds simple, right? The helper module contains fake. In the current implementation I mostly ignored how fake "knows" which methods exists. Then there is this tiny detail called jsdocs verification tests, that I need to adjust as well. Then there are naming conflicts with module methods e.g. lorem.word and word.word. Should the module classes already start using the standalone methods? TLDR: Consider this a POC. I transformed the entire code base, to see which problems we need to handle. For an in depth review, I can split this PR later into one per module, but we likely cannot do any releases once the first one lands until all of them are in (unless you don't make them public api). FYI: Existing tests are passing now, but we need to consider how we change/duplicate them, to ensure both the standalone and the faker tree based ones have a desent coverage. |
|
Interesting finding with createFakerCore: return {
definitions: Array.isArray(definitions)
? mergeLocales(definitions)
: (definitions ?? {}),
randomizer: randomizer ?? ({} as Randomizer),
config: config ?? {},
}-> Some side effect, causes vs return {
definitions: Array.isArray(definitions)
? definitions[0]
: (definitions ?? {}),
randomizer: randomizer ?? ({} as Randomizer),
config: config ?? {},
}-> Not included mergeLocales doesn't even use/reference simpleFaker! (Same with generateMersenne53Randomizer) 🤔 WHY!? |
Thanks for the in depth explanation. I was already aware about the "person => helper => number" situation. That why I honestly had the number module in mind for this experiment. I was not aware of the faker.helper.fake situation. |
d802170 to
6fbd6d0
Compare
|
I switched from |
|
I just noticed that my current implementation of a moduleRegistry for fake leads to a circular dependency. Depending on your point of view all methods that use fake experience a breaking change. For now, I call all internal fake methods in the least breaking way, while still allowing for some level of tree shaking. Due to circular dependencies it is probably impossible to implememt in an entirely non-breaking way. We might be able to relieve some of the pain points by switching from fake patterns to resolver functions internally. But that would be a breaking change and a separate PR for sure. See also I'll push the latest version once I get the fake issue resolved. |
72e1d00 to
c3b50eb
Compare
c3b50eb to
61b4844
Compare
|
It's working... |
fantastic 🚀 is it possible to clean this PR up and/or spliterate it into smaller PR-chunks? e.g. extract preparing chores which does not affect directly the implementation of this PR, but is required for it e.g. I see some utilities like casing-utils (which I usually do by https://www.npmjs.com/package/change-case), also some file namings now with a prefixed underscore |
|
As for cleaning this PR, not sure how to do that (except for squashing a few commits here and there). I would like to split this PR into smaller pieces, but not sure whether some commits needs to be kept together. e.g.
Before we start merging a bunch of stuff, there are a few things that we need to consider:
|
PR #3748 Review: Standalone Module Functions
SummaryThis PR introduces a fundamental architectural shift from class-based module methods to standalone functions, enabling tree-shaking and smaller bundle sizes. The approach is sound - the ~30% reduction in bundle size for targeted imports is compelling. I've done a deep review of the core infrastructure, the transformation pattern, and the resulting code. Below are my findings, concerns, and suggestions. Architecture & DesignThe overall architecture is clean and well-thought-out: The I especially like how the per-function mini-registries for Critical Issues1.
|
|
@Shinigami92 Sorry for the question, but is that your opinion/an analysis backed by you personally or just a copy and paste from Claude's analysis of this PR? |
second |
|
I'm fine with reviews from AI tools, just add a hint that this does or does not reflect your personal opinions. I'm aware of all/most of the points brought up by the review, thats why I asked for explicit opinions here: |
ah now I understand where your comment was coming from 👍 very good that you are aware of almost everything claude discovered |
|
FYI: I updated the FakerCore PR: It uses a slightly different implementation then the one used in this PR but both are generally compatible. This/SMF PR:
FakerCore PR:
|


Implements #2667
The goal of this PR is to convert our class/module based system to one, that uses standalone module functions and use them as the implementation of modules.
Non-goals of this PR:
This PR will be created/achieve in the following steps:
faker.person.firstName()vsfirstName(englishCore)vsfirstName(nanoCore)generate-module-treescript, that rebuilds the modules using the standalone functionsUpdate docs generation script to handle the new structureHow to review:
Partial alternatives