feat: introduce NumberRange type#3838
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #3838 +/- ##
==========================================
+ Coverage 98.84% 98.90% +0.06%
==========================================
Files 899 899
Lines 3113 3105 -8
Branches 574 550 -24
==========================================
- Hits 3077 3071 -6
+ Misses 32 30 -2
Partials 4 4
🚀 New features to boost your workflow:
|
|
@ST-DDT @xDivisionByZerox — looking for input on a tradeoff this PR introduces. ContextThis PR replaces ~40 inline Options1. Per-concept named rangesDefine small types per use-case, each carrying its own JSDoc: export interface WordCount extends NumberRange {
/** The minimum number of words to generate. */
min: number;
/** The maximum number of words to generate. */
max: number;
}
2. Hybrid: inline shape for surface methods,
|
|
IMO The most important benefit of this change is the shortened method signature => simplified readability. There are a few questions that we need to search answers for first: A) Do our users understand that a range is {min, max} and NOT [min, max] with minimal effort?
B) Do our users have a need for types or do they inline the arguments anyway? C) Is the context closeness of numberRange.min/max important to our users?
Order of preference: 3, 5, 2, 1, 4 Diff-Preview The current apidoc generation process does not display. Min and max params anymore:
Note: when the range currently has a default for either value, then we no longer display it in the jsdocs after this PR is merged.
Isn't the whole point of this issue/PR to rethink this? If not, what is it? |
|
@ST-DDT thanks for the feedback 👍
The most valueable feature for consumers would be to use import { type NumberRange, faker} from '@faker-js/faker';
const options: NumberRange = {
// ^^^^^^^^^^^
min: 1,
max: 10
};
// vvvvvvv passing typed options
faker.module.method1(options)
faker.module.method2(options)
faker.module.method3(options)So thinking about this, it would indeed be the best option to use Option 1, even when this is a bit verbose on our side. |
|
The workaround right now is something very complex like: import { faker } from '@faker-js/faker';
type NumberRange = Exclude<Parameters<faker.module.method>[0], number>pseudo code, not tested |
Why do you think this distinction is important for our users? const options: NumberRange = {
min: 1,
max: 10
};
faker.module.method1(options)
faker.module.method2(options)
faker.module.method3(options)vs const options: WordCountRange = {
min: 1,
max: 10
};
faker.module.method1(options)
faker.module.method2(options)
faker.module.method3(options)IMO both types are identical. And adding more imports/different types just increases the mental costs to remember how they are defined. |
|
You are right on the first look, but when in an IDE the second one ( |






--> DRAFTING <--