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

Replace new Date().setFullYear(...) with new Date(Date.UTC(...)) #377

Closed
Shinigami92 opened this issue Jan 30, 2022 · 4 comments · Fixed by #343
Closed

Replace new Date().setFullYear(...) with new Date(Date.UTC(...)) #377

Shinigami92 opened this issue Jan 30, 2022 · 4 comments · Fixed by #343
Assignees
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@Shinigami92
Copy link
Member

Describe the bug

We have some code parts in the codebase that resets the year, month and date part of a Date but not the time part.
These are not safe and deterministic.

Reproduction

new Date().setFullYear(2100, 1, 1) !== new Date(Date.UTC(2100, 0))

Additional Info

Example:

options.min = new Date().setFullYear(1990, 1, 1);

@Shinigami92 Shinigami92 added the s: pending triage Pending Triage label Jan 30, 2022
@Shinigami92 Shinigami92 added this to the v6.1 - First bugfixes milestone Jan 30, 2022
@Shinigami92 Shinigami92 moved this to Todo in Faker Roadmap Jan 30, 2022
@Shinigami92 Shinigami92 added the c: bug Something isn't working label Jan 30, 2022
@github-actions github-actions bot removed the s: pending triage Pending Triage label Jan 30, 2022
@samnap11
Copy link

Is there a typo in the reproduction? Is this what you mean?
new Date().setFullYear(2100, 0, 1) !== new Date(Date.UTC(2100, 0))?

Nevertheless, the issue still stands. I can take this issue and work on the fix. Is that fine?

@Shinigami92
Copy link
Member Author

Ah yeah, month is 0 based, sry

There are already some open issue related to the date module. Maybe it's a good idea to wait until these are merged so no heavy merge conflicts are happening.

@Shinigami92
Copy link
Member Author

#343 solves it in src code, but date.spec.ts still use setFullYear 🤔

Also waiting on #200 and #576 before doing anything to reduce conflicts

@Shinigami92 Shinigami92 added the s: on hold Blocked by something or frozen to avoid conflicts label Mar 23, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 23, 2022

The tests don't need to be changed because they use relative dates whereas datetime uses a static boundary.
So IMO #343 fixes this issue fully.

@Shinigami92 Shinigami92 linked a pull request Mar 24, 2022 that will close this issue
@Shinigami92 Shinigami92 moved this from Todo to Awaiting Review in Faker Roadmap Mar 24, 2022
@Shinigami92 Shinigami92 added p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug and removed s: on hold Blocked by something or frozen to avoid conflicts labels Mar 24, 2022
Repository owner moved this from Awaiting Review to Done in Faker Roadmap Mar 24, 2022
@ST-DDT ST-DDT removed this from Faker Roadmap Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants