WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.4k stars 4.16k forks source link

`@wordpress/date`: Moving away from `moment` and the role of the package #43533

Open ciampo opened 2 years ago

ciampo commented 2 years ago

With PR #43005 merged, the @wordpress/components package replaced its dependency on moment with date-fns.

Since moment and moment-timezone are large dependencies, it would be great if we could migrate away from them entirely. The only package left requiring them is @wordpress/date.

I believe that, in order to decide on the next steps, we need to answer two questions:

What should the role of the @wordpress/date package be?

Potential approaches:

  1. @wordpress/date as the centralized datetime-utils library of Gutenberg. All other packages requiring datetime-related utilities would import @wordpress/date
  2. (as suggested by @noisysocks in https://github.com/WordPress/gutenberg/pull/43005#discussion_r952040389) @wordpress/date as complimentary to a date util library (e.g. date-fns), which should only provide:
    • A store for the WordPress site's date settings including format preference and timezone.
    • A function similar to wp_date in PHP.\

As suggested by @gziolo in https://github.com/WordPress/gutenberg/issues/43533#issuecomment-1224363999, in order to minimize the chance of introducing breaking changes, we could keep the @wordpress/date untouched, mark it as deprecated, and potentially create a new package instead (following any of the approaches proposed above).

How should we replace moment ?

In the components package, so far moment has been partially replaced with date-fns (partially, because internationalization/formatting for each locale and timezones are still handled via @wordpress/date, which internally uses moment).

For the future, it looks like the Temporal APIs may be a great fit (as suggested by @griffbrad and @sgomes in https://github.com/WordPress/gutenberg/pull/41385#issuecomment-1144712585)

gziolo commented 2 years ago

Great idea. We tried something similar before. Unfortunately, the previous attempt to replace moment was more difficult than we anticipated:

@wordpress/data is probably used by plugins. I would start with verifying that. We might need to keep the old API for backward compatibility. The alternative would be deprecating the package in favor of something else.

ciampo commented 2 years ago

Great idea. We tried something similar before. Unfortunately, the previous attempt to replace moment was more difficult than we anticipated:

Cc'ing @vcanales and @swissspidy who collaborated on the previous attempt

noisysocks commented 2 years ago

I wouldn't mind working on this, but not until after 6.1. Iā€™m a little bit familiar with @wordpress/date from refactoring DateTimePicker and working on #39670.

noisysocks commented 2 years ago

@wordpress/date as the centralized datetime-utils library of Gutenberg. All other packages requiring datetime-related utilities would import @wordpress/date

I doubt we'd design a better API than date-fns or Temporal so with this approach @wordpress/date would likely become a wrapper around an existing library similar to how @wordpress/element is a wrapper around React. The advantage of this approach is that we can change the underlying library in the future without breaking backwards compatibility, but, to be honest, I think YAGNI!

(as suggested by @noisysocks in https://github.com/WordPress/gutenberg/pull/43005#discussion_r952040389) @wordpress/date as complimentary to a date util library (e.g. date-fns), which should only provide:

  • A store for the WordPress site's date settings including format preference and timezone.
  • A function similar to wp_date in PHP.\

Yeah at this stage this is my preferred approach. Let me elaborate a little bit.

Basically Gutenberg and third party plugins should use date-fns or moment or Temporal or Date or whatever they choose for manipulating dates. There are advantages to each.

Then, @wordpress/date only needs to contain two things:

1) A JavaScript implementation of wp_date. We need this so that we can interpret arbitrary PHP format strings as selected by the user in the settings page:

Screen Shot 2022-08-24 at 10 31 29

2) A store for the Timezone, Date Format, Time Format and Week Starts On site settings as selected by the user in the settings page:

Screen Shot 2022-08-24 at 10 30 14

We could store these settings in editor settings instead but I think it's convenient having them in @wordpress/date so that non-editor parts of WordPress can enqueue wp-date and have this functionality.

noisysocks commented 2 years ago

Regarding Temporal: I think it's a great fit but not ready just yet. Right now it will increase our bundle size because the polyfill defines classes (e.g. Temporal.Instant) that can't be tree shaken as effectively as the pure functions in date-fns. Also the authors do not currently recommend using Temporal in production. When a big browser ships Temporal and we don't need the polyfill as often then I think it will be very compelling.

sgomes commented 2 years ago

Thank your for all your work on this, @noisysocks! šŸ‘

Regarding Temporal, I agree that it may be a bit too early to move in that direction given the current state of the polyfill, unfortunately. But it seems that we're in agreement that we should move to it eventually, once support is there.

As such, the decision we should make, as I see it, is whether we want @wordpress/date to serve as an abstraction layer that would survive that future transition by reworking its internals while maintaining the same external API; or whether we want to keep it small, make our code directly dependant on date-fns, and then simply rewrite all date-fns-dependant code when the time comes (like we're now planning to rewrite all moment-dependant code).

Neither of those options is a clear choice for me, to be honest šŸ˜•

The former is good from an engineering perspective, but it involves a lot of upfront work and loses much of its value once the migration to Temporal is complete, as it's unlikely future migrations will be needed. It can also fail to account for some use-cases and thus require continued future maintenance and improvement.

The latter saves us a bunch of upfront work, but punts it to the future, and assumes there will be interest and availability in migrating away from date-fns when the time is right, without making that eventual transition easier in any way.

Tough choice šŸ˜•

noisysocks commented 2 years ago

Well put @sgomes!

There are only two parts of the entire codebase that have ever used moment: @wordpress/date and DateTimePicker. DateTimePicker now uses date-fns. So it's not actually a lot of code that needs to be migrated.

Also I don't think there will be any urgency to migrate from date-fns to Temporal as date-fns is a very lightweight tree-shakeable set of functions that uses Date which is a browser API that's never going anywhere. In other words I think it's no big deal to put off the second migration for many years, perhaps indefinitely.

So the only value we would get from creating a wrapper around moment / date-fns / Temporal (similar to @wordpress/element) is for third party plugins that currently use the window.moment object added by WordPress.

We're going to have to continue to support window.moment regardless of what we do so that we don't break plugins which use this.

If we create a wrapper, we will be encouraging plugins to switch from window.moment to window.wp.date. We'll swap one global that needs to remain backwards compatible for a different global that needs to remain backwards compatible.

If, however, plugins switch to date-fns, then that makes maintenance easier as date-fns doesn't add anything to window. Plugins can use their own copy of date-fns (or just use Date) and update their version of date-fns or switch to Temporal at their own pace independent of Core.

tl;dr: I agree with your framing of cost of maintenance vs. cost of migration way of framing the issue but think that the cost of maintenance is very high and the cost of migration is fairly low šŸ˜€

sgomes commented 2 years ago

We're going to have to continue to support window.moment regardless of what we do so that we don't break plugins which use this.

Can we simply require these plugins to enqueue moment themselves, even if it is a breaking change? We can still ship the library, just not load it by default.

Having an implicit window.moment is a problem, as we're massively increasing our API surface area, and worse, delegating it to an external provider over which we have no control. Removing window.moment by default seems like an acceptable thing to do with a major version / breaking change, as long as it's easy to explicitly request it if you need it.

Ideally, the only endpoints we'd expose would be our own, to provide some level of insulation against changes in implementation details, such as libraries or native features used.

date-fns doesn't add anything to window. Plugins can use their own copy of date-fns (or just use Date) and update their version of date-fns or switch to Temporal at their own pace independent of Core.

This is a great approach that solves the above, going forward šŸ‘

noisysocks commented 2 years ago

Yeah definitely something to explore. Probably something to discuss in a Core JS meeting. I know we've been flexible in the past about backwards compatibility when it comes to deprecated JavaScript libraries. For instance we "broke backwards compatibility" by upgrading jQuery but did it in a fairly slow methodical way.