Date: Update moment-timezone package to support string timezones
#22866
Conversation
|
Size Change: 0 B Total Size: 1.2 MB ℹ️ View Unchanged
|
|
I think ideally, we shouldn't rely on moment and find an alternative. We'd save both timezone bundle sizes and the library's bundle size too (as it's a huge one). I know it's not an easy task but if I remember properly we tried to avoid making it visible in the public API so it should be possible. The fact that moment is a global that any plugin can tweak (change timezone...) is problematic. |
|
@youknowriad has there been past discussion on alternatives? I agree there are enough downsides to moment that merit a switch, but for now, updating the version here would still be good to fix those timezone issues in the plugin. |
not a lot but the ones that come in discussions from time to time are date-fns or luxon
Updating the version is fine. What is concerning for me is the bundle-size increase. moment-timezone is huge and ideally, we should try to avoid loading these. |
|
Have you considered passing in our own timezone data? Moment offers a 10-year bundle, we could split out that timezone data and load it in Something like this maybe: add/timezone-tooltip...try/add-timezone-data |
|
Speaking just from the original purpose of this PR, I just wanted to catch Gutenberg up to core's moment version, because it fixed the timezone problem in #22193, and although it increased the bundle size of GB, wouldn't change wp core. I don't know about importing in a file that core folks will need to keep updated, if we're going in that direction I feel like getting away from moment entirely would be better. I see there was a PHP solution on #23400, I'll check that out to see if it also fixes the original issue that spawned this PR. |
This is interesting, I thought we made sure Core don't load moment-timezone either cc @gziolo otherwise there's no point in preventing it just in the plugin. And I'd definitely be in favor of moving away from moment if possible, that lib is doing more harm than good. |
|
I've been thinking about this and while I think we should definitely move away from moment, it doesn't make sense to keep the plugin lagging behind Core's version and if the optimization is just present on the plugin, it loses its value. So we should ensure we bring back the optim for both Core and the plugin separately. |
youknowriad
force-pushed
the
update/date-moment-timezone
branch
from
ef5735c
to
31b93fd
Compare
|
Looks like this PR broke the Gutenberg storybook https://wordpress.github.io/gutenberg/ |
Description
Fixes #22193. Sites with string timezones throw errors because the timezone is not recognized by moment. This issue isn't present in core, because the moment-timezone used in core is a higher version, which correctly loads all the timezones.
I moved the required version all the way to current,
0.5.31.@gziolo This undoes the optimization from #12356, bringing the built date size back up to ~200K… but it seems we do need these timezones defined after all (see #22193 (comment))
How has this been tested?
The unit tests still pass, this was only an issue in the browser. Change your site to use a string timezone by picking a city from the site options dropdown. Load the editor, add a Latest Posts block, and toggle on the date display. On master, you'll get a series of console errors about the date. Switch to this branch, install & build, they should be fixed.
Alternately, check the timezones moment supports with
moment.tz.names(). On master, onlyWPis shown.Types of changes
Bug fix (non-breaking change which fixes an issue)