Skip to content
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(date): introduce global refDate instead of passing it every time #859

Closed
king-in-it opened this issue Apr 21, 2022 · 7 comments · Fixed by #1757
Closed

feat(date): introduce global refDate instead of passing it every time #859

king-in-it opened this issue Apr 21, 2022 · 7 comments · Fixed by #1757
Assignees
Labels
c: feature Request for new feature m: date Something is referring to the date module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Milestone

Comments

@king-in-it
Copy link

Describe the bug

When using faker.seed with a value, e.g. faker.seed(123), faker should always give back the same values for the faker functions, but this is not the case for faker.date.past(). It uses a fixed amount of time to go back, but that's always the current date minus that fixed time. So for example calling faker.date.past() today would give a different result when calling it tomorrow.

The issue is not present when providing the parameters maxYears and refDate, e.g. faker.date.past(30, new Date(2022,0)).

Reproduction

  1. Set the faker seed: faker.seed(123);
  2. Call faker.date.past() (without parameters)
  3. Rerunning the same code provides a different value for that same faker.date.past()

See https://stackblitz.com/edit/typescript-qiclti for a reproduction.

Additional Info

@faker-js/faker version used: 6.1.2

@king-in-it king-in-it added the s: pending triage Pending Triage label Apr 21, 2022
@Shinigami92
Copy link
Member

This is because the refDate is default to Date.now() as the docs say.
Not sure if this is a bug, just the default params 🤔

@king-in-it
Copy link
Author

Indeed, it has to do with calling the toDate function (https://github.com/faker-js/faker/blob/main/src/date.ts#L47) with the refDate being undefined, which causes the refDate to be a new Date() (

date = new Date();
). I'm using the faker.seed to give predictable results (e.g. in conjunction with Jest snapshots), but using faker.date.past() with current implementation is unpredictable...

@ST-DDT
Copy link
Member

ST-DDT commented Apr 21, 2022

IMO this is not a bug. With a fixed refDate it is predictable.

If you pass a refDate yourself and do result.getTime() - refDate.getTime() then this will always result in the same value for a given seed.
We use now as a default to ensure the value actually contains dates that are "soon/future" (relative to now/refDate) whenever you are calling it.
As for the jest snapshots, I think you can avoid the issue with property matchers:
https://jestjs.io/docs/snapshot-testing#property-matchers

@king-in-it
Copy link
Author

Thanks for the snapshots tip, but I don't want to set this manually in each snapshot whenever a date is involved. That's why the faker.seed is useful. Also the documentation states: "If you want consistent results, you can set your own seed", but if faker.date.past() changes each time, that is not very consistent. When a seed is set, then instead of assigning new Date() perhaps the Date can be initialized by other values of the faker lib to have a fixed ref date?

@ST-DDT
Copy link
Member

ST-DDT commented Apr 21, 2022

When a seed is set, then instead of assigning new Date() perhaps the Date can be initialized by other values of the faker lib to have a fixed ref date?

We could do that of course or even hardcode it. However we have to weight two different concerns here.
Getting expected values without parameters and context, getting fixed values. (Both are reproducible)

If you are calling faker.date.future() and you get 1970-01-01T13:37:42Z I guest most users would be surprised.
IMO having a parameter that gives you control over your "expectation" of future/past is a good way to get good values.

If you are interested in explicitly setting the refDate "globally" instead of passing it to the method every time, then you could open a PR for that.

@ST-DDT ST-DDT added c: feature Request for new feature s: awaiting more info Additional information are requested and removed s: pending triage Pending Triage labels Apr 21, 2022
@ST-DDT ST-DDT changed the title faker.date.past() not static when using faker.seed feat(date): introduce global refDate instead of passing it every time Apr 21, 2022
@king-in-it
Copy link
Author

Setting the refDate globally would be a good solution for this, because then this can be set near the faker.seed function call! Thanks for all the input!

@Shinigami92 Shinigami92 removed the s: awaiting more info Additional information are requested label Apr 21, 2022
@JontyMC
Copy link

JontyMC commented Jun 1, 2022

+1

@xDivisionByZerox xDivisionByZerox added the m: date Something is referring to the date module label Jul 29, 2022
MH4GF added a commit to MH4GF/graphql-codegen-typescript-mock-data that referenced this issue Sep 25, 2022
As shown in the following code, the result changes even if the seed is added:
```
> const { faker } = require("@faker-js/faker")
undefined
> faker.seed(0)
0
> const a = faker.date.past().toISOString()
undefined
> a
'2022-03-08T21:45:15.158Z'
> faker.seed(0)
0
> const b = faker.date.past().toISOString()
undefined
> b
'2022-03-08T21:45:30.411Z'
> a === b
false
```

This is because the default refDate passed in the second argument of faker.date.past() is now, so we can fix it at a specific date.

ref: faker-js/faker#859
MH4GF added a commit to MH4GF/graphql-codegen-typescript-mock-data that referenced this issue Sep 28, 2022
As shown in the following code, the result changes even if the seed is added:
```
> const { faker } = require("@faker-js/faker")
undefined
> faker.seed(0)
0
> const a = faker.date.past().toISOString()
undefined
> a
'2022-03-08T21:45:15.158Z'
> faker.seed(0)
0
> const b = faker.date.past().toISOString()
undefined
> b
'2022-03-08T21:45:30.411Z'
> a === b
false
```

This is because the default refDate passed in the second argument of faker.date.past() is now, so we can fix it at a specific date.

ref: faker-js/faker#859
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Jan 18, 2023
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug p: 1-normal Nothing urgent labels Jan 18, 2023
@ST-DDT ST-DDT self-assigned this Jan 18, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Faker Roadmap Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature m: date Something is referring to the date module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants