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

Partial for #3252: new Date() -> Date.now() #3314

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jkriegshauser
Copy link
Contributor

[x] Updated Changelog
[x] Partial fix for #3252 (documentation is in a separate repo)

Btw, this probably shouldn't be in the PR template as 100+ unrelated files changed:

  1. Please run npm run lint:prettier before submitting so that
    style issues are fixed.

I can remove it from the template .md if you want. Or do a separate PR with all the style issues fixed :P

@jkriegshauser
Copy link
Contributor Author

Also, would be nice if we had a way to enforce people not using new Date() in new code, maybe a change to pull-request rules, or an automated test that scans the code? I'm open to ideas.

@KristjanESPERANTO
Copy link
Contributor

would be nice if we had a way to enforce people not using new Date() in new code

The prefer-date-now rule of the unicorn ESLint plugin seems to be a good solution for that.

@sdetweil
Copy link
Collaborator

sdetweil commented Dec 31, 2023

but teaching others the right way is a long haul problem.

everybody has to quit breaking old code.

@jkriegshauser
Copy link
Contributor Author

would be nice if we had a way to enforce people not using new Date() in new code

The prefer-date-now rule of the unicorn ESLint plugin seems to be a good solution for that.

oh that's cool, this would be ideal. I'm not sure how to hook it up though.

@@ -95,7 +95,7 @@ WeatherProvider.register("weatherbit", {
// Implement WeatherDay generator.
generateWeatherDayFromCurrentWeather (currentWeatherData) {
//Calculate TZ Offset and invert to convert Sunrise/Sunset times to Local
const d = new Date();
const d = Date.now();
let tzOffset = d.getTimezoneOffset();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Date.now() returns the number of milliseconds since January 1, 1970. methods like getTimezoneOffset will not work with your change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or those in calendarUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Fixed up in latest push

@jkriegshauser
Copy link
Contributor Author

@KristjanESPERANTO @sdetweil I could use some help with this. In calendar.js, this works:

	createEventList (limitNumberOfEntries) {
		const ONE_SECOND = 1000; // 1,000 milliseconds
		const ONE_MINUTE = ONE_SECOND * 60;
		const ONE_HOUR = ONE_MINUTE * 60;
		const ONE_DAY = ONE_HOUR * 24;

		const now = new Date(); // new Date(Date.now()); // breaks tests
		const today = moment().startOf("day");
		const future = moment().startOf("day").add(this.config.maximumNumberOfDays, "days").toDate();
		let events = [];

But when I change this part:

		const now = new Date(Date.now()); // breaks tests
		const today = moment(Date.now()).startOf("day");
		const future = moment(Date.now()).startOf("day").add(this.config.maximumNumberOfDays, "days").toDate();

then some of the electron tests like has css class dayBeforeYesterday hang and time out after 20 sec. It's pretty easy to repro, but I'm not sure how to debug the electron tests.

@khassel
Copy link
Collaborator

khassel commented Jan 1, 2024

I'm not sure if the current construction is o.k. but I can explain what happens.

If you change the one line then Date.now() is already mocked in calendar.js which is not the case with the old line.

The mocking happens in tests/electron/helpers/global-setup.js.

The tests time out because the expected css property never appears.

@KristjanESPERANTO
Copy link
Contributor

I don't have a better suggestion yet, but somehow new Date(Date.now()) seems a bit too hacky to me. Is there perhaps a more elegant approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants