Should we remove moment dependency? (arv) #769
Comments
See wikimedia-gadgets#769 for the rationale, but tl;dr the only uses of [momentjs](https://momentjs.com/) are in `twinklearv` and are cosmetic, so loading moment is wasteful. There are two functions we need to replace, `.format()` and `.calendar()`; this uses `.toLocaleString()` and `.toLocaleDateString()`, respectively. Regarding formatting, doing: var localeOptions = { hour: 'numeric', hour12: false, minute: 'numeric', day: 'numeric', month: 'long', year: 'numeric', timeZone: 'UTC', timeZoneName: 'short' }; new Date().toLocaleString(undefined, localeOptions) produces: November 25, 2019, 21:36 UTC This doesn't fully recpitulate `moment().utc().format('HH:mm, D MMMM YYYY [(UTC)]')`, which produces: 21:36, 25 November 2019 (UTC) The difference is negligible, and although it was chosen (0beacc0) to match on-wiki formats, that's a user preference and, notably, *not* a valid date format (see wikimedia-gadgets#757). A more exact reproduction can be achieved by combining some strings, doing something like: var localeTimeOptions = { timeStyle: 'short', hour12: false, timeZone: 'UTC' }; var d = new Date(); d.toLocaleTimeString(undefined, localeTimeOptions) + ', ' + d.toLocaleDateString(undefined, {day: 'numeric', timeZone: 'UTC'}) + ' ' + new Date().toLocaleDateString(undefined, {month: 'long', year: 'numeric', timeZone: 'UTC'}) + ' (UTC)'; We could place this in a function, but hardly seems necessary, especially since, depending on their selected user preference, not all users are used to the same formats. As for the relative dates from the `.calendar()` function, they simply can't be easily replaced. This implementation uses `.toLocaleDateString()` to produce `Fri, Nov 22, 2019, 2:03 PM`, whereas `.calendar()` would have produced `Last Friday at 2:03 PM`. The short month and weekday names are used to declutter the window as compared to a long string.
|
I first mentioned this in #723 (comment) where @siddharthavp said:
(also cc @MusikAnimal, who commented there) Another option would be to use the CDN (à la #692) for |
Or how about just writing a lightweight date library ourselves, customised for our needs? We could put it in Morebits, so that it can use the ResourceLoader. |
|
I was under the impression that dependencies aren't "downloaded" more than once, no matter how many gadgets use them. As you say, MediaWiki Core uses moment, so if my belief is true, then we shouldn't be adding much weight in using it ourselves. Or maybe I've had this wrong all along? |
|
moment was recently removed from the MediaViewer. Now, according to codesearch linked on phab, moment is being used only in |
|
@MusikAnimal: That's right, but moment is not currently loaded by default. I didn't know about @siddharthvp's answer re: MediaViewer, but I started noticing this this fall, so that would make perfect sense to me.
Although also unnecessary since we don't actually have any needs atm? |
Isn't #770 basically a need? Also, a date library can be used for better ways of writing code like this and this. I did write Morebits.date after I left the comment above. The main highlights are:
Most of this is moment-inspired. Don't know what you think about it, but I guess this would make for a fine addition to Morebits? (also ping @MusikAnimal for feedback). Many parts are probably overkill (like |
|
FWIW, my point about a need was just that #770's display is really a choice. Sure, it's a spectrum — we need to not show At any rate @siddharthvp, that I think it's likely I won't get around to reviewing and incorporating it this month but maybe next round? How's that sound? I'd be inclined to just merge #770 to lighten the load in the mean time, but I guess it's not urgent. Really great work here, truly appreciate it. 🦄 |
New class in Morebits for handling of dates per brief discussion in/after wikimedia-gadgets#769 (comment). This will replace the moment dependency. Some of the functionality here exceeds that in moment - the `format` function can read the user's preferred time zone from the site preferences, which is ideal since dates shown by MediaWiki core and extensions are in that time zone. All the various UTC/non-UTC abbrev/full combinations for getting month/day names are provided. I didn't particularly like doing this, but I guess it's good to have them all for the sake of completeness. Some of these get used in `format`. The `format` function has been reworked. The [gist version](https://gist.github.com/siddharthvp/dcdc51e05aa79a5856e63ecc116dcacf) presented a couple of problems with substitution. The rewrite should be more efficient and robust, and we get to implement `calendar` in terms of `format`. `Date.prototype.getUTCMonthName` and `Date.prototype.getUTCMonthNameAbbrev` have been removed (in favour of equivalent functions in Morebits.date). This is a good thing to do as modifications of native prototypes is discouraged in modern JavaScript. For i18n, only the `Morebits.date.localeData` object needs to be tweaked.
New class in Morebits for handling of dates per brief discussion in/after wikimedia-gadgets#769 (comment). This will replace the moment dependency. Some of the functionality here exceeds that in moment - the `format` function can read the user's preferred time zone from the site preferences, which is ideal since dates shown by MediaWiki core and extensions are in that time zone. All the various UTC/non-UTC abbrev/full combinations for getting month/day names are provided. I didn't particularly like doing this, but I guess it's good to have them all for the sake of completeness. Some of these get used in `format`. The `format` function has been reworked. The [gist version](https://gist.github.com/siddharthvp/dcdc51e05aa79a5856e63ecc116dcacf) presented a couple of problems with substitution. The rewrite should be more efficient and robust, and we get to implement `calendar` in terms of `format`. `Date.prototype.getUTCMonthName` and `Date.prototype.getUTCMonthNameAbbrev` have been removed (in favour of equivalent functions in Morebits.date). This is a good thing to do as modifications of native prototypes is discouraged in modern JavaScript. For i18n, only the `Morebits.date.localeData` object needs to be tweaked.
Amorymeltzer
added
the
deprecations
Amorymeltzer
added
the
gadget-definition
New class in Morebits for handling of dates per brief discussion in/after wikimedia-gadgets#769 (comment). This will replace the moment dependency. Some of the functionality here exceeds that in moment - the `format` function can read the user's preferred time zone from the site preferences, which is ideal since dates shown by MediaWiki core and extensions are in that time zone. All the various UTC/non-UTC abbrev/full combinations for getting month/day names are provided. I didn't particularly like doing this, but I guess it's good to have them all for the sake of completeness. Some of these get used in `format`. The `format` function has been reworked. The [gist version](https://gist.github.com/siddharthvp/dcdc51e05aa79a5856e63ecc116dcacf) presented a couple of problems with substitution. The rewrite should be more efficient and robust, and we get to implement `calendar` in terms of `format`. `Date.prototype.getUTCMonthName` and `Date.prototype.getUTCMonthNameAbbrev` have been removed (in favour of equivalent functions in Morebits.date). This is a good thing to do as modifications of native prototypes is discouraged in modern JavaScript. For i18n, only the `Morebits.date.localeData` object needs to be tweaked.
Added in a2c3f93 / 0beacc0 and originally loaded as a Twinkle module, it has since #249 been a simple dependency via the gadget definition.
Currently, it is used only for a single module,
twinklearv.js, and even then only for single segment: reporting to WP:AN3. In the past month, WP:AN3 has averaged about 5 reports every 3 days (i.e. there have been just under 50 reports in 31 days). The formatting stuff is replaceable, so only the relative language from .calendar("Today at 7PM") would be lost. The specific display (see #770 for proposed implementation) isn't particularly important here, but the upshot is that momentjs is (1) barely used and (2) definitely not necessary.I'm not sure how caches are treated on phones or tablets, but on my laptop it looks pretty generous. Still, moment is somewhere around 8% of the full-sized Twinkle package (per inspect), and just under 10% as actually loaded (that's 86.8 and 23.4 kb, respectively). It leads to fewer than two edits a day, so it's fairly unnecessary right now and just adds some weight and complexity. There's probably a future for moment here as regards i18n, but it isn't now.
The text was updated successfully, but these errors were encountered: