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.85% 98.84% -0.01%
==========================================
Files 923 923
Lines 3224 3216 -8
Branches 591 583 -8
==========================================
- Hits 3187 3179 -8
Misses 33 33
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,
|
Shinigami92
added
s: needs decision
|
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 ( |
|
IMO in order to move this forward, I would suggest starting with one type and if users complain, add another one. |
Shinigami92
force-pushed
the
feat-add-NumberRange-type
branch
from
c4a4376 to
9b2f1d3
Compare
Decision: locking in Option 3 (single shared
|
There was a problem hiding this comment.
Currently the NumberRange type is resolved as a type name in the docs, instead of "exploding" the type declaration into more primitive types:
If we want t change that and instead show the underlying "object"-type we would need to resolve it via it's symbol (type => symbol => declaration [type=alias] => type node => text). We do not seem to have tests for parameters so I wasn't able to test this out of the box.
Edit: Should be somewhere here where we would need to change that:
faker/scripts/apidocs/processing/type.ts
Lines 145 to 156 in fd33b72
Here the docs patch (click to expand)diff --git a/scripts/apidocs/processing/type.ts b/scripts/apidocs/processing/type.ts
index 25ec30c96..2c0310e20 100644
--- a/scripts/apidocs/processing/type.ts
+++ b/scripts/apidocs/processing/type.ts
@@ -91,6 +91,8 @@ export function getTypeText(
: newSimpleType('string');
return newUnionType([displayType, baseType]);
+ } else if (name === 'NumberRange') {
+ return newSimpleType('{ min: number; max: number }');
}
const typeParameters = typeArguments.map((t) => getTypeText(t, options));
diff --git a/test/scripts/apidocs/__snapshots__/method.spec.ts.snap b/test/scripts/apidocs/__snapshots__/method.spec.ts.snap
index 7be83ab71..e85e9c37c 100644
--- a/test/scripts/apidocs/__snapshots__/method.spec.ts.snap
+++ b/test/scripts/apidocs/__snapshots__/method.spec.ts.snap
@@ -20,6 +20,7 @@ exports[`method > expected and actual methods are equal 1`] = `
"methodWithThrows",
"multiParamMethod",
"noParamMethod",
+ "numberRangeParamMethod",
"optionalStringParamMethod",
"optionsInlineParamMethodWithDefaults",
"optionsInterfaceParamMethodWithDefaults",
@@ -342,7 +343,7 @@ exports[`method > processMethodLike(literalUnionParamMethod) 1`] = `
},
{
"default": undefined,
- "description": "Value \`'a'\` or \`'b'\` or an array thereof.",
+ "description": "\`'a'\` or \`'b'\` or an array thereof.",
"name": "mixed",
"type": {
"text": "'a' | 'b' | string | Array<'a' | 'b' | string>",
@@ -395,7 +396,7 @@ exports[`method > processMethodLike(literalUnionParamMethod) 1`] = `
},
{
"default": undefined,
- "description": "Value \`'a'\` or \`'b'\` or an array thereof.",
+ "description": "\`'a'\` or \`'b'\` or an array thereof.",
"name": "namedMixed",
"type": {
"text": "'a' | 'b' | string | Array<AB | string>",
@@ -983,6 +984,111 @@ exports[`method > processMethodLike(noParamMethod) 1`] = `
}
`;
+exports[`method > processMethodLike(numberRangeParamMethod) 1`] = `
+{
+ "name": "numberRangeParamMethod",
+ "signatures": [
+ {
+ "deprecated": undefined,
+ "description": "Test with NumberRange.",
+ "examples": [],
+ "experimental": undefined,
+ "parameters": [
+ {
+ "default": undefined,
+ "description": "\`{min: 1, max: 10}\`.",
+ "name": "value",
+ "type": {
+ "text": "{ min: number; max: number }",
+ "type": "simple",
+ },
+ },
+ {
+ "default": undefined,
+ "description": "Array of \`{min: 1, max: 10}\`.",
+ "name": "array",
+ "type": {
+ "text": "Array<{ min: number; max: number }>",
+ "type": "generic",
+ "typeParameters": [
+ {
+ "text": "{ min: number; max: number }",
+ "type": "simple",
+ },
+ ],
+ },
+ },
+ {
+ "default": undefined,
+ "description": "The options parameter.",
+ "name": "options",
+ "type": {
+ "text": "{ ... }",
+ "type": "simple",
+ },
+ },
+ {
+ "default": undefined,
+ "description": "The count parameter.",
+ "name": "options.count",
+ "type": {
+ "text": "{ min: number; max: number }",
+ "type": "simple",
+ },
+ },
+ {
+ "default": undefined,
+ "description": "\`{min: 1, max: 10}\` or an array thereof.",
+ "name": "mixed",
+ "type": {
+ "text": "{ min: number; max: number } | Array<{ min: number; max: number }>",
+ "type": "union",
+ "types": [
+ {
+ "text": "{ min: number; max: number }",
+ "type": "simple",
+ },
+ {
+ "text": "Array<{ min: number; max: number }>",
+ "type": "generic",
+ "typeParameters": [
+ {
+ "text": "{ min: number; max: number }",
+ "type": "simple",
+ },
+ ],
+ },
+ ],
+ },
+ },
+ ],
+ "remarks": [],
+ "returns": {
+ "text": "string",
+ "type": "simple",
+ },
+ "seeAlsos": [],
+ "signature": "function numberRangeParamMethod(
+ value: NumberRange,
+ array: ReadonlyArray<NumberRange>,
+ options: {
+ /** The count parameter. */
+ count: NumberRange;
+ },
+ mixed: NumberRange | ReadonlyArray<NumberRange>
+ ): string;",
+ "since": "1.0.0",
+ "throws": [],
+ },
+ ],
+ "source": {
+ "column": -1,
+ "filePath": "test/scripts/apidocs/method.example.ts",
+ "line": -1,
+ },
+}
+`;
+
exports[`method > processMethodLike(optionalStringParamMethod) 1`] = `
{
"name": "optionalStringParamMethod",
diff --git a/test/scripts/apidocs/method.example.ts b/test/scripts/apidocs/method.example.ts
index 1651129e9..3aeebb86e 100644
--- a/test/scripts/apidocs/method.example.ts
+++ b/test/scripts/apidocs/method.example.ts
@@ -1,4 +1,4 @@
-import type { Casing, ColorFormat } from '../../../src';
+import type { Casing, ColorFormat, NumberRange } from '../../../src';
import { FakerError } from '../../../src/errors/faker-error';
import type { LiteralUnion } from '../../../src/internal/types';
import type { AlphaNumericChar } from '../../../src/modules/string';
@@ -182,8 +182,8 @@ export class SignatureTest {
* @param namedValue `'a'` or `'b'`.
* @param array Array of `'a'` or `'b'`.
* @param namedArray Array of `'a'` or `'b'`.
- * @param mixed Value `'a'` or `'b'` or an array thereof.
- * @param namedMixed Value `'a'` or `'b'` or an array thereof.
+ * @param mixed `'a'` or `'b'` or an array thereof.
+ * @param namedMixed `'a'` or `'b'` or an array thereof.
*
* @since 1.0.0
*/
@@ -205,6 +205,34 @@ export class SignatureTest {
);
}
+ /**
+ * Test with NumberRange.
+ *
+ * @param value `{min: 1, max: 10}`.
+ * @param array Array of `{min: 1, max: 10}`.
+ * @param options The options parameter.
+ * @param options.count `{min: 1, max: 10}`.
+ * @param mixed `{min: 1, max: 10}` or an array thereof.
+ *
+ * @since 1.0.0
+ */
+ numberRangeParamMethod(
+ value: NumberRange,
+ array: ReadonlyArray<NumberRange>,
+ options: {
+ /** The count parameter. */
+ count: NumberRange;
+ },
+ mixed: NumberRange | ReadonlyArray<NumberRange>
+ ): string {
+ return JSON.stringify({
+ value,
+ array,
+ options,
+ mixed,
+ });
+ }
+
/**
* Test with a Record parameter.
*Feel free to apply now or in a separate PR. |
Shinigami92
dismissed stale reviews from xDivisionByZerox and ST-DDT
via
9648390
|
@ST-DDT example: https://deploy-preview-3838.fakerjs.dev/api/airline.html#flightnumber |
3 participants
the following PR description is claude generated
Summary
Introduces a shared, publicly exported
NumberRangeinterface and uses it to replace the ~40 inline{ min: number; max: number }shapes scattered across the module method signatures.Refs #1443.
Motivation
number | { min; max }object now read asnumber | NumberRange.NumberRangevalue and pass it to any range-accepting method, instead of re-declaring the shape inline or reverse-engineering it viaParameters<...>.Design decision (Option 3)
The discussion weighed several approaches (a single shared type vs. per-concept named types such as
WordCountRange, hybrid inline/shared shapes, and intersection types). We are going with Option 3: a single sharedNumberRange.Rationale:
NumberRangeand a hypotheticalWordCountRangeare structurally identical - extra per-concept types mostly add mental overhead and import churn.Tradeoffs
min/max(e.g. "The minimum number of words to generate.") is replaced by the genericNumberRangefield docs ("The minimum value (inclusive)."). The method-level@param x.min/@param x.maxlines are unaffected.min/maxsub-parameters (and their per-field defaults) for parameters typed asNumberRange. This needs to be addressed separately in the apidoc generation - see the discussion thread.