moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.33k stars 730 forks source link

weekNumber getters and similar getters, updates object #1518

Open rol-jacob opened 1 year ago

rol-jacob commented 1 year ago

Describe the bug When using luxon in frontend libraries like Vue.js and setting a luxon object as a reactive variable. Reading e.g. .weekNumber from the luxon object will trigger watchers. To Reproduce Please share a minimal code example that triggers the problem:

import { reactive, watch } from 'vue';
import { DateTime } from 'luxon';

const luxonObj = reactive(DateTime.now());
watch(luxonObj, () => {
  console.log(luxonObj.weekNumber) // Reading weekNumber will fire watcher twice.
});

Actual vs Expected behavior Reading a number should not write to object model if not necessary.

Desktop (please complete the following information):

diesieben07 commented 1 year ago

Luxon uses caching in various places to speed up slow operations, like computing the week information or (more importantly) Intl operations (which are often notoriously slow). In theory we could avoid using the DateTime object for this cache and instead use a WeakMap and make DateTime truly immutable. I am not sure on the browser support for WeakMap though.

dasa commented 1 year ago

I am not sure on the browser support for WeakMap though.

It's quite good: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap#browser_compatibility

MonstraG commented 9 months ago

Yep, 97%, even ie11 supports it: https://caniuse.com/mdn-javascript_builtins_weakmap

diesieben07 commented 9 months ago

I looked into it and unfortunately using WeakMap is only about half as fast as a plain property on the DateTime object. You can see my code here: https://github.com/moment/luxon/compare/master...diesieben07:luxon:issue/1518/caching-improvements Benchmarking results:

DateTime#weekday x 168,758,367 ops/sec ±1.71% (89 runs sampled)
DateTime#weekday old cache x 306,887,551 ops/sec ±0.40% (97 runs sampled)
DateTime#weekday no cache x 3,159,972 ops/sec ±0.35% (99 runs sampled)

As you can see from the "no cache" entry, not using a cache here is not really viable. Any other suggestions are welcome.

G-Rath commented 4 months ago

I'm working on a Vue app myself that makes heavy use of weekNumber and co, and have found that they're surprisingly slow - I'm just wondering if there is anything blocking using WeakMap since it sounded like it was well enough supported and did give a significant performance boost even if it's not as fast as plain properties?

diesieben07 commented 4 months ago

The thing blocking the use of WeakMap is that it is twice as slow as the current option.