feat(datatype)!: reimplement datatype module

[Shinigami92]

#1590 (comment)

This implements all methods that can be compared against the typeof operator

[ST-DDT]

If the purpose of the method is typeof value === 'function', then this is unnecessary.
If you think differently please explain why this is more useful than returing a null, string, number or even an empty object.

[ST-DDT]

In a different method chose new Date(). Why not return object() or any()/unknown().

[ST-DDT]

You could litterally implement this via
return helpers.objectValue(this).

[Shinigami92]

re-check pls

[ST-DDT]

This still doesn't explain what is special about Promise.resolve().

[Shinigami92]

I enhanced the jsdoc a bit to make more clear that the return value of such a function can be empty, throw an error, return a promise or return a value of any type.
This is helpful when you e.g. just want to generate a function that will be passed to a generic function accepting a callback.
That way within a randomized deterministic test case you will find out via faker that you miss a case for e.g. handling promises and or correctly handle error cases if such a callback would throw something.

I know that users out there should cover such cases on specific base, but for example if you come in to a not so good maintained project and introduce faker, you could e.g. start with such a generic way and from there you can then find out e.g. "oh we missed to handle promise cases".

[ST-DDT]

Also maybe dont sort the methods for this PR to reduce the diff.
Separate PR.

[codecov]

Codecov Report

Attention: Patch coverage is 86.45418% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 99.59%. Comparing base (9882760) to head (4fa9a18).
Report is 87 commits behind head on next.

❗ Current head 4fa9a18 differs from pull request most recent head 6e66d9a. Consider uploading reports for the commit 6e66d9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2044      +/-   ##
==========================================
+ Coverage   99.55%   99.59%   +0.04%     
==========================================
  Files        2817     2801      -16     
  Lines      251203   252578    +1375     
  Branches     1125     1121       -4     
==========================================
+ Hits       250080   251562    +1482     
+ Misses       1094      989     -105     
+ Partials       29       27       -2     

Files	Coverage Δ
src/modules/number/index.ts	100.00% <100.00%> (ø)
src/modules/string/index.ts	100.00% <100.00%> (ø)
src/modules/datatype/index.ts	94.48% <86.06%> (-5.52%)	⬇️

... and 182 files with indirect coverage changes

[Shinigami92]

Also maybe dont sort the methods for this PR to reduce the diff. Separate PR.

This is done explicitly
We have two sections

1. the typeof in sorted order
2. the old deprecated methods (order not changed)

I suggest to just read/review the new code above the // deprecated methods comment
everything below did not change and was not touched

[matthewmayer]

Any of the methods which previously had a working options parameter in 7.4 but are now ignored e.g. array, number should be documented in the Upgrading to v8 guide as breaking changes.

[ST-DDT]

This is the same as returning undefined / the undefined method reference.

[Shinigami92]

We should not try to be efficient and fancy here and indirectly knowing that the undefined case surprisingly covers the empty/void case
while reading the internal code it is more clear that we have several cases we want to cover:

1. void -- the simpliest case of all
2. an "unknown" whatever return value
3. an Error
4. a Promise that is successful
5. a Promise that is not successful

[ST-DDT]

What about a promise that is not yet resolved?

[Shinigami92]

What about a promise that is not yet resolved?

Interesting 🤔

It looks like new Promise(() => {}) does return immediately,
so we need a sleep like this

new Promise((resolve) => {
  setTimeout(resolve, 2**31 - 1);
});

side note: https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#maximum_delay_value
setTimeout has a max delay #TIL

[Shinigami92]

For some of the suggestions, lets talk tomorrow when we do the v8.0 dev-weekend

[matthewmayer]

The new implementations of these methods seem so different from the old methods that it feels confusing to keep them with the same names.

I feel if someone proposed "a method which returns a Number, but it could be NaN or POSITIVE_INFINITY so its only useful for checking with typeof" as a new issue it would be tagged as "waiting for user interest". So does it make sense to implement these very limited methods if we dont know if users actually need them?

[Shinigami92]

@ST-DDT in the meeting notes we also had defined unknown(): any of the datatype methods (TS "unknown")
Can we outsource this to an issue and mark as awaiting for user interest?

[Shinigami92]

The new implementations of these methods seem so different from the old methods that it feels confusing to keep them with the same names.

We know that, but sadly there is no other option as these methods are explicitly named for what the typeof operator supports

I feel if someone proposed "a method which returns a Number, but it could be NaN or POSITIVE_INFINITY so its only useful for checking with typeof" as a new issue it would be tagged as "waiting for user interest". So does it make sense to implement these very limited methods if we dont know if users actually need them?

Yes this was a team decision on 13. April 2023.

Reasons:

• We dont want to drop the datatype module because we could not find another place for boolean
• Then we came up with the idea to make the datatype module valuable for the typeof operator

Now it is possible to e.g. use faker.datatype.number() to pass to a function that just accepts a number.
The test can then check that anykind of number, so also NaN or Infinity, are handled correctly by that function.
This should be mostly (or even only) the case when e.g. you go into a already existing project and introduce faker. Then you can write generic tests with these datatype methods and talk to you project lead and tell them to enhance their error handling or TypeScript definitions (if they are using TypeScript at all).

---

Edit 1:
@matthewmayer now you know the reasons behind, you are very good in pointing out to us when something is missing in the docs
So it would be very welcome if you could help me with that 🙂

[matthewmayer]

Wouldn't it make the migration path much easier if new names were used for the new methods

For example
faker.datatype.numberType instead of faker.datatype.number?

[ST-DDT]

I also think, that we might want to move this to v9.0 to let the deprecation sink in for one mayor version, before replacing the methods from one version to another with completely different ones.
(or use new method names as @matthewmayer suggested)

[ST-DDT]

I think we have to put more thought into this as this isn't really usable with TS.

function test() {
  toTest(faker.datatype.function()); // Type-Error
  nestedCall(faker.datatype.unknown()); // Type-Error
}

function toTest(fn: () => number): number {
  return nestedCall(fn());
}

function nestedCall(value: number): number {
  return value + 1;
}

We should either reduce this to the most minimal viable implementation: return () => ?
Or give this more options, such as should this throw?
What do I expect as result? Passing the result of this method as an argument violates the genericity of the definition and thus behaves the same as using unknown.
This might be a viable test case for plain JS applications, but even then the utility of that seems pretty limited to me.
I assume it most cases passing faker.datatype.number as a reference would produce better results.

I could imagine that something like this might produce more useful results:

function<ResultType = unknown, PromiseWrapped extends boolean>( options: {
  errorRate: number;
  returnType: `${ResultType}`
  promise: PromiseWrapped 
}): PromiseWrapped extends true ? Promise<ResultType> : ResultType;

But since this is what I came up with in just an hour, there might be more options that should be considered.
For now, we should stick to the minimal implementation or omit the method entirely with some docs stating for function you should use the other methods and pass them as a reference directly or build one yourself using them or other methods as basis.
That way we follow the "Open (for extensions and) Closed (for modifications)" principle and breaking any workflows that depend on the temporary behavior.

[github-actions]

Uncommitted changes were detected after runnning generate:* commands.
Please run pnpm run preflight to generate/update the related files, and commit them.

[Shinigami92]

I'm thinking about to move this to v10, because in v9 the deprecated methods will be removed. In v10 we could be sure that all users are moved away from these old methods, because they where not available at all anymore.

Another idea would be to define a new module that makes the new aim more clear, like faker.typeof.number, faker.typeof.string and so on 🤔

[matthewmayer]

I would be in favour of a different module name.

[matthewmayer]

Although I think "typeof" is a reserved word so it would have to be something else.

[Shinigami92]

Although I think "typeof" is a reserved word so it would have to be something else.

If I would try to, TS shows:
'typeof' is not allowed as a variable declaration name. ts(1389)

[matthewmayer]

Why two positive infinities?

[matthewmayer]

How about faker.type.*

[ST-DDT]

This is probably no longer blocked (but needs a rebase and some thoughts).