fix: improved fine-grainability of ReactiveDate #12110

FoHoOV · 2024-06-20T13:00:32Z

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

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
Prefix your PR title with feat:, fix:, chore:, or docs:.
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests and linting

Run the tests with pnpm test and lint the project with pnpm lint

changeset-bot · 2024-06-20T13:00:36Z

🦋 Changeset detected

Latest commit: ac907f0

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 · 2024-06-20T14:40:05Z

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 · 2024-06-20T15:06:23Z

@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 · 2024-06-21T20:21:08Z

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 · 2024-06-24T19:22:56Z

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 · 2024-06-25T07:27:50Z

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 · 2024-06-25T17:25:54Z

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 · 2024-06-27T06:53:47Z

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

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

I guess its ok now? >_<

trueadm · 2024-06-27T08:06:06Z

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

for setX and setUTCX It uses the same signal

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 · 2024-06-27T08:17:27Z

@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 · 2024-06-27T08:26:40Z

@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.

packages/svelte/src/reactivity/date.js

Rich-Harris

This looks good now but 5 tests fail locally for me — lots of off-by-one errors owing to the timezone I'm in. Pretty sure we just need to tweak the tests, but it's eluding me

FoHoOV · 2024-07-03T14:06:58Z

@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):

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();
});

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 · 2024-07-03T16:31:54Z

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!

.changeset/sleepy-dogs-sit.md

FoHoOV · 2024-07-03T16:51:34Z

@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 >_<

FoHoOV marked this pull request as ready for review June 20, 2024 13:12

FoHoOV added 8 commits June 27, 2024 10:20

fix: improve ReactiveDate class

7df85f4

improved test

dc55523

added changeset

52c362f

improved performance by using less signals

b937500

using the same signal for getX and getUTCX

e0c417e

fixed types

408ade5

combined setX with setUTCX tests together

b9e9e4e

simplified fine-grainability by using deriveds

0703693

FoHoOV force-pushed the improve-reactive-date branch from 38a33da to 0703693 Compare June 27, 2024 06:51

vercel bot deployed to Preview – svelte-5-preview June 27, 2024 08:28 View deployment

vercel bot deployed to Preview – svelte-dev June 27, 2024 08:28 View deployment

trueadm reviewed Jun 27, 2024

View reviewed changes

packages/svelte/src/reactivity/date.js Outdated Show resolved Hide resolved

trueadm reviewed Jun 27, 2024

View reviewed changes

packages/svelte/src/reactivity/date.js Outdated Show resolved Hide resolved

trueadm reviewed Jun 27, 2024

View reviewed changes

packages/svelte/src/reactivity/date.js Outdated Show resolved Hide resolved

FoHoOV added 2 commits June 28, 2024 10:41

reused the calculated new_time

e274dda

removed logic that reduced number of signals, cuz its use-cases are rare

81f9df6

FoHoOV requested a review from trueadm June 28, 2024 09:10

inference is fine

09226bb

Rich-Harris added 5 commits June 29, 2024 17:32

tidy up

a7ee760

tweaks

36f9cdb

more efficient iteration

8bb3dca

tweak

ba30938

more descriptive changeset

69f4612

Rich-Harris requested changes Jun 29, 2024

View reviewed changes

Rich-Harris added 3 commits June 29, 2024 18:00

this makes all tests but one pass

0ec966b

merge main

f130567

fix

af373a3

Rich-Harris added 2 commits July 3, 2024 12:13

adjust test to be robust across timezones

7b0f1b7

tweak more tests

bbfdb14

Rich-Harris reviewed Jul 3, 2024

View reviewed changes

.changeset/sleepy-dogs-sit.md Outdated Show resolved Hide resolved

vercel bot deployed to Preview – svelte-5-preview July 3, 2024 16:34 View deployment

vercel bot deployed to Preview – svelte-dev July 3, 2024 16:35 View deployment

Update .changeset/sleepy-dogs-sit.md

ac907f0

Rich-Harris approved these changes Jul 3, 2024

View reviewed changes

Rich-Harris merged commit e5d70c3 into sveltejs:main Jul 3, 2024
7 of 9 checks passed

github-actions bot mentioned this pull request Jul 3, 2024

Version Packages (next) #12282

Merged

FoHoOV deleted the improve-reactive-date branch July 3, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improved fine-grainability of ReactiveDate #12110

fix: improved fine-grainability of ReactiveDate #12110

FoHoOV commented Jun 20, 2024

changeset-bot bot commented Jun 20, 2024 •

edited

Loading

Rich-Harris commented Jun 20, 2024

FoHoOV commented Jun 20, 2024 •

edited

Loading

7nik commented Jun 21, 2024

trueadm commented Jun 24, 2024

FoHoOV commented Jun 25, 2024 •

edited

Loading

trueadm commented Jun 25, 2024 •

edited

Loading

FoHoOV commented Jun 27, 2024 •

edited

Loading

trueadm commented Jun 27, 2024

FoHoOV commented Jun 27, 2024

trueadm commented Jun 27, 2024

Rich-Harris left a comment

FoHoOV commented Jul 3, 2024 •

edited

Loading

Rich-Harris commented Jul 3, 2024

FoHoOV commented Jul 3, 2024

fix: improved fine-grainability of ReactiveDate #12110

fix: improved fine-grainability of ReactiveDate #12110

Conversation

FoHoOV commented Jun 20, 2024

Svelte 5 rewrite

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

Tests and linting

changeset-bot bot commented Jun 20, 2024 • edited Loading

🦋 Changeset detected

Rich-Harris commented Jun 20, 2024

FoHoOV commented Jun 20, 2024 • edited Loading

7nik commented Jun 21, 2024

trueadm commented Jun 24, 2024

FoHoOV commented Jun 25, 2024 • edited Loading

trueadm commented Jun 25, 2024 • edited Loading

FoHoOV commented Jun 27, 2024 • edited Loading

trueadm commented Jun 27, 2024

FoHoOV commented Jun 27, 2024

trueadm commented Jun 27, 2024

Rich-Harris left a comment

Choose a reason for hiding this comment

FoHoOV commented Jul 3, 2024 • edited Loading

Rich-Harris commented Jul 3, 2024

FoHoOV commented Jul 3, 2024

changeset-bot bot commented Jun 20, 2024 •

edited

Loading

FoHoOV commented Jun 20, 2024 •

edited

Loading

FoHoOV commented Jun 25, 2024 •

edited

Loading

trueadm commented Jun 25, 2024 •

edited

Loading

FoHoOV commented Jun 27, 2024 •

edited

Loading

FoHoOV commented Jul 3, 2024 •

edited

Loading