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(datatype)!: reimplement datatype module #2044
base: next
Are you sure you want to change the base?
Conversation
}, | ||
{ | ||
weight: 1, | ||
value: () => Promise.resolve(), |
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.
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.
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.
In a different method chose new Date()
. Why not return object()
or any()
/unknown()
.
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.
You could litterally implement this via
return helpers.objectValue(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.
re-check pls
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.
This still doesn't explain what is special about Promise.resolve()
.
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 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".
Also maybe dont sort the methods for this PR to reduce the diff. |
Codecov ReportAttention: Patch coverage is
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
|
This is done explicitly
I suggest to just read/review the new code above the |
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.
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.
value: () => { | ||
// empty function | ||
}, |
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.
This is the same as returning undefined
/ the undefined
method reference.
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.
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:
- void -- the simpliest case of all
- an "unknown" whatever return value
- an Error
- a Promise that is successful
- a Promise that is not successful
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.
What about a promise that is not yet resolved?
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.
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
For some of the suggestions, lets talk tomorrow when we do the v8.0 dev-weekend |
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? |
@ST-DDT in the meeting notes we also had defined |
We know that, but sadly there is no other option as these methods are explicitly named for what the
Yes this was a team decision on 13. April 2023. Reasons:
Now it is possible to e.g. use Edit 1: |
Wouldn't it make the migration path much easier if new names were used for the new methods For example |
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 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)
src/modules/datatype/index.ts
Outdated
* | ||
* @since 8.0.0 | ||
*/ | ||
function(): () => unknown { |
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 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.
b57df32
to
bb8dfa2
Compare
bb8dfa2
to
6c9bced
Compare
ed8cafe
to
4fa9a18
Compare
4fa9a18
to
0709aec
Compare
Uncommitted changes were detected after runnning |
0709aec
to
a5db12f
Compare
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 |
I would be in favour of a different module name. |
Although I think "typeof" is a reserved word so it would have to be something else. |
If I would try to, TS shows: |
{ weight: 1, value: () => this.faker.number.float() }, | ||
{ weight: 1, value: () => this.faker.number.int() }, | ||
{ weight: 1, value: () => Number.NaN }, | ||
{ weight: 1, value: () => Number.POSITIVE_INFINITY }, |
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.
Why two positive infinities?
How about faker.type.* |
This is probably no longer blocked (but needs a rebase and some thoughts). |
#1590 (comment)
This implements all methods that can be compared against the
typeof
operator