fix: improved fine-grainability of ReactiveDate

[FoHoOV]

Currently any changes to ReactiveDate class causes every read method to be notified, this should be making them more fine-grained. This PR also adds tests for Date class (none UI).

fixed Date part of #11727

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• [x] Prefix your PR title with feat:, fix:, chore:, or docs:.
• [x] This message body should clearly illustrate what problems it solves.
• [x] Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• [x] Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: ac907f0f69dc128420e38746a8fc68fb6d7a6f11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

This is a pretty significant increase in complexity that adds performance and memory overhead. I'm not personally convinced it's worth it — fine-grained-ness is an optimisation (i.e. it's not a correctness issue), and as such if it makes common cases less optimal then it's probably something that should be avoided. If it's important to avoid invalidating something, it's easy enough to do it like this:

let year = $derived(date.getFullYear());

[FoHoOV]

@Rich-Harris made it a tad more performant, but if it still doesn't worth feel free to close it. Also just wanted to mention that it will only create signals when the method is actually called so we don't create signals on initialization.

Edit: I feel like if we think this way, lots of things aren't fine-grained in reactivity package, so can we just remove it and let the user create one? cuz these kinds of behaviors breaks my expectations of what reactive date or set means; or add these behaviors to docs so people are aware?

Edit2: Also it introduces inconsistencies between these classes. For instance Set.add and Set.delete will be fine-grained for Set.has (not always which is another inconsistency), but Date.setHours is not? IMHO they all should behave the same all the time.

[7nik]

I don't really think it's required. I think of Date as a timestamp wrapper with a bunch of methods to format it, while Set, Map, and URLSearchParams are collections.

Also, you or somebody else can always create a package with super fine-grained native types.

[trueadm]

What if we instead just keep mostly what we have today already, but instead of reading only the time value, we read the time value as part of a derived that also returns the full value. Then wouldn't each API be fine-grain whilst not needing much in the way of anything? Date is kind of unique in that you normally only listen to a handful of things, so creating a fresh derived each time is likely the least expensive and least complicated thing to do.

[FoHoOV]

What if we instead just keep mostly what we have today already, but instead of reading only the time value, we read the time value as part of a derived that also returns the full value. Then wouldn't each API be fine-grain whilst not needing much in the way of anything? Date is kind of unique in that you normally only listen to a handful of things, so creating a fresh derived each time is likely the least expensive and least complicated thing to do.

If I undertood you correctly, then I don't think that would be the case. If you do date.setSeconds(69) it will change date.getMinutes and date.getSeconds (and sometimes date.getHoures, etc.) but it will not affect other readers. The time has changed but for example the date.getYear is still the same. I also want to emphsize on my last comment (edit1 and edit2) what are your thoughts on that?

[trueadm]

What if we instead just keep mostly what we have today already, but instead of reading only the time value, we read the time value as part of a derived that also returns the full value. Then wouldn't each API be fine-grain whilst not needing much in the way of anything? Date is kind of unique in that you normally only listen to a handful of things, so creating a fresh derived each time is likely the least expensive and least complicated thing to do.

If I undertood you correctly, then I don't think that would be the case. If you do date.setSeconds(69) it will change date.getMinutes and date.getSeconds (and sometimes date.getHoures, etc.) but it will not affect other readers. The time has changed but for example the date.getYear is still the same. I also want to emphsize on my last comment (edit1 and edit2) what are your thoughts on that?

What if we instead just keep mostly what we have today already, but instead of reading only the time value, we read the time value as part of a derived that also returns the full value. Then wouldn't each API be fine-grain whilst not needing much in the way of anything? Date is kind of unique in that you normally only listen to a handful of things, so creating a fresh derived each time is likely the least expensive and least complicated thing to do.

If I undertood you correctly, then I don't think that would be the case. If you do date.setSeconds(69) it will change date.getMinutes and date.getSeconds (and sometimes date.getHoures, etc.) but it will not affect other readers. The time has changed but for example the date.getYear is still the same. I also want to emphsize on my last comment (edit1 and edit2) what are your thoughts on that?

I mean it would create a separate derived signal and return that from doing getSeconds and getMinutes. Thus when you change the seconds, then if the minutes hasn't changed, then it won't flag it.

i.e.

for (const method of read) {
	// @ts-ignore
	proto[method] = function (...args) {
		const derived_value = derived(() => {
			get(this.#raw_time);
			// @ts-ignore
			return date_proto[method].apply(this, args);
		});

		return get(derived_value);
	};
}

[FoHoOV]

I mean it would create a separate derived signal and return that from doing getSeconds and getMinutes. Thus when you change the seconds, then if the minutes hasn't changed, then it won't flag it.

i.e.

for (const method of read) {
	// @ts-ignore
	proto[method] = function (...args) {
		const derived_value = derived(() => {
			get(this.#raw_time);
			// @ts-ignore
			return date_proto[method].apply(this, args);
		});

		return get(derived_value);
	};
}

Ow now I get it :D It would greatly simplify the logic. I stole your idea (bowahaha) and merged it with mine which uses less signals

1. for setX and setUTCX It uses the same signal
2. for things that only depend on raw_time (called versioned_signals here) it create a single shared signal

I guess its ok now? >_<

[trueadm]

I mean it would create a separate derived signal and return that from doing getSeconds and getMinutes. Thus when you change the seconds, then if the minutes hasn't changed, then it won't flag it. i.e.

for (const method of read) {
	// @ts-ignore
	proto[method] = function (...args) {
		const derived_value = derived(() => {
			get(this.#raw_time);
			// @ts-ignore
			return date_proto[method].apply(this, args);
		});

		return get(derived_value);
	};
}

Ow now I get it :D It would greatly simplify the logic. I stole your idea (bowahaha) and merged it with mine which uses less signals

1. for setX and setUTCX It uses the same signal
2. for things that only depend on raw_time (called versioned_signals here) it create a single shared signal

I guess its ok now? >_<

You definitely don't want to store them in a map, otherwise you'll just retain too much. Just create a new one each time the method is called.

[FoHoOV]

@trueadm out of curiosity, doesn't that lead to creating more signals? If I call getX in 10 places it would create 10 signals (if I'm understanding how derived works correctly), after the last review, I thought we wanted to create the least amount of signals possible.

[trueadm]

@FoHoOV It would but deriveds clean themselves up. However, I think you're also right in that we probably want to reduce the amount of deriveds being created.

[FoHoOV]

@Rich-Harris @trueadm Sorry I was away for a couple of days. The issue is funny actually xD I hate Date deeply. lets say for inital_date we do:

const initial_date = new Date('2023-01-01T00:00:00.000+0000');

calling the getX methods (not getUTCX methods) will give the time back according to your timezone (the initial date is created as UTC with +0000 so it has no offset, but that doesn't matter). so basically if your timezone is -05:00 then calling date.getFullYear() will give you 2022. So when we set date.setFullYear(b.getFullYear()), b.getFullYear() might differ from date.getFullYear because of time offsets (same goes for other methods). Lets take this one as an example:

test('date.setDate and date.setUTCDate', () => {
	const date = new SvelteDate(initial_date);
	const log: any = [];

	const cleanup = effect_root(() => {
		render_effect(() => {
			log.push(date.getMonth());
		});
		render_effect(() => {
			log.push(date.getUTCMonth());
		});
	});

	flushSync(() => {
		date.setMonth(initial_date.getMonth() + 5);
	});

	flushSync(() => {
		date.setMonth(initial_date.getMonth() + 5); // no change expected
	});

	flushSync(() => {
		date.setUTCMonth(initial_date.getUTCMonth() + 10);
	});

	assert.deepEqual(log, [
		// something
	]);

	cleanup();
});

if you timezone is lets say -08:00 (US & Canada) then initial date will be in Dec 2022, then when we call date.setMonth(initial_date.getMonth() + 5) date will be May 2023. The last setUTCMonth will not see the year has changed, and everything fails.

There are multiple ways to overcome this issue, but I've come into two solutions which don't hack around how Date and timeZones work (like for instance you could set the date to 2022-6-15 so the timezone doesn't affect the year but, maybe there are other edge-cases that I can't see for that and it really feels hacky):

1. first solution (test each setX method individually):

const initial_date = new Date('2023-01-01T00:00:00.000+0000');

function apply_change_to_date(time: number, set_method_name: DateMethods<'set'>, value: number) {
	const date = new Date(time);
	date[set_method_name](value);
	return date;
}

test('date.getMonth and date.setUTCMonth', () => {
	const date = new SvelteDate(initial_date);
	let expected: { utc: number | null; local: number | null } = {
		local: initial_date.getMonth(),
		utc: initial_date.getUTCMonth()
	};

	const cleanup = effect_root(() => {
		render_effect(() => {
			assert.equal(expected.local, date.getMonth());
			expected.local = null;
		});
		render_effect(() => {
			assert.equal(expected.utc, date.getUTCMonth());
			expected.utc = null;
		});
		render_effect(() => {
			assert.isNull(expected.local);
			assert.isNull(expected.utc);
		});
	});

	flushSync(() => {
		const new_value = initial_date.getMonth() + 5;
		const new_date = apply_change_to_date(initial_date.getTime(), 'setMonth', new_value);
		expected = { local: new_date.getMonth(), utc: new_date.getUTCMonth() };
		date.setMonth(new_value);
	});

	flushSync(() => {
		const new_value = initial_date.getMonth() + 5;
		const new_date = apply_change_to_date(initial_date.getTime(), 'setMonth', new_value);
		expected = { local: new_date.getMonth(), utc: new_date.getUTCMonth() };
		date.setMonth(new_value);
	});

	flushSync(() => {
		const new_value = initial_date.getUTCMonth() + 10;
		const new_date = apply_change_to_date(date.getTime(), 'setUTCMonth', new_value);
		expected = { local: new_date.getMonth(), utc: new_date.getUTCMonth() };
		date.setUTCMonth(new_value);
	});

	cleanup();
});

2. second solution (test all setX methods together)

test('date.setX and date.setUTCX', () => {
	const date_prop_names = Object.getOwnPropertyNames(Date.prototype);
	const utc_set_methods = date_prop_names.filter((name) =>
		name.startsWith('setUTC')
	) as unknown as Array<DateMethods<'setUTC'>>;

	utc_set_methods.forEach((utc_set_method) => {
		const svelte_date = new SvelteDate(initial_date);

		const utc_get_method = utc_set_method.replace('set', 'get') as unknown as DateMethods<'getUTC'>;
		const local_set_method = utc_set_method.replace('UTC', '') as unknown as DateMethods<
			'set',
			'UTC'
		>;
		const local_get_method = utc_get_method.replace('UTC', '') as unknown as DateMethods<
			'get',
			'UTC'
		>;

		let expected: { utc: number | null; local: number | null } = {
			utc: initial_date[utc_get_method](),
			local: initial_date[local_get_method]()
		};

		const cleanup = effect_root(() => {
			render_effect(() => {
				const value = svelte_date[utc_get_method]();
				assert.equal(value, expected.utc, `in effect for ${utc_get_method}`);
				expected.utc = null;
			});

			render_effect(() => {
				const value = svelte_date[local_get_method]();
				assert.equal(value, expected.local, `in effect for ${local_get_method}`);
				expected.local = null;
			});
		});

		render_effect(() => {
			// expect both local and utc to be null at point, because
			// if they they have a value at this point, then it means the effect,
			// didn't run for them
			assert.isNull(expected.local);
			assert.isNull(expected.utc);
		});

		flushSync(() => {
			const new_value = initial_date[local_get_method]() + 5;
			const new_date = apply_change_to_date(initial_date.getTime(), local_set_method, new_value);
			expected = { local: new_date[local_get_method](), utc: new_date[utc_get_method]() };
			svelte_date[local_set_method](new_value);
		});

		flushSync(() => {
			const new_value = initial_date[local_get_method]() + 5;
			svelte_date[local_set_method](new_value);
			// no rerun expected here
		});

		flushSync(() => {
			const new_value = initial_date[utc_get_method]() + 10;
			const new_date = apply_change_to_date(svelte_date.getTime(), utc_set_method, new_value);
			expected = { local: new_date[local_get_method](), utc: new_date[utc_get_method]() };
			svelte_date[utc_set_method](new_value);
		});

		cleanup();
	});
});

I tested both of them on many different time-zones. If you liked the solution I can apply the changes >_< - You guys always come up with something 1000x simpler 🗡️

[Rich-Harris]

I just took another run at this — I adjusted the last couple of tests that weren't passing (earlier I had changed the sample dates from new Date('2023-01-01T00:00:00.000+0000') to new Date(2023, 0, 1, 0, 0, 0, 0) to make them timezone-agnostic). Checked with a dozen or so timezones, I think we're good. Thanks!

[FoHoOV]

@Rich-Harris creating dates like that is actually based on local timezone, here is the MDN reference (see "Individual date and time component values" part.

The parameter values are all evaluated against the local time zone, rather than UTC.

Why it works is because of the initial date:

new Date(2023, 0, 2, 0, 0, 0, 0);

Since it starts at day 2 of year 2023, then for instance "-12:00" will not change the month or year, so it will work. I thought changing the intial value feels like a hack (as mentioned in the comment above). It seems there are no edge cases that will fail because of this :D Thanks for your "date" and time >_<