Date package: do not bundle moment.js for getSettings only - #76865

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: alshakero <alshakero@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Size Change: +27 B (0%)

Total Size: 7.74 MB

_{compressed-size-action}

Flaky tests detected in ca02876.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24036439391
📝 Reported issues:

• [Flaky Test] post title and content are editable and blocks can be inserted #76524 in /test/e2e/specs/editor/various/post-content-focus-mode.spec.js
• [Flaky Test] should show correct homepage actions based on current homepage or posts page #68892 in /test/e2e/specs/site-editor/homepage-settings.spec.js

I think it makes sense to split up these files a bit more for general maintainability and separation of concerns. 👍

On the overall approach, I don't love using a subpath export for this and I wish we could just have a better default experience. Would another way to go about this be to isolate the side effects into their own file(s) so that tree-shaking the root index.js could avoid the side-effect (e.g. add sideEffects to package.json that contain the side-effecty files)?

I also wonder: With the recent changes in #75831, do we even need the top-level setupWPTimezone ? My understanding of the point of that pull request was that each function would be responsible for ensuring that everything is "set up", which feels like a better approach than a top-level side-effect. cc @jsnajdr @epeicher . I understand there are different functions here (setupWPTimezone and ensureWPTimezone) but the fundamental premise would be some call-time idempotent guarantee that the setup has happened.

@aduth I thought about this for a moment, and I think it won't be possible for me to achieve keeping the exported surface the same without a breaking change. Currently, importing the library in any way runs these side effects, and these side effects require moment.js. This behavior is basically part of the API of the library, and stopping it is a breaking change.

This behavior is basically part of the API of the library, and stopping it is a breaking change.

But if I understand, the behavior is an expected part of the API through how it's used by the package's exported functions. So if those functions were to instead ensure the setup happens (has happened) lazily when called, they behave the same as they have previously. This is what I was expecting out of the ensureWPTimezone changes in #75831, or at least seems like a similar idea.

In fact, if I follow the code, setupWPTimezone should already be called by these functions anyways because of how they use ensureWPTimezone. Can't we just remove the module-level setupWPTimezone at the bottom of the file and mark the package as sideEffects: false ?

In fact, if I follow the code, setupWPTimezone should already be called by these functions anyways because of how they use ensureWPTimezone. Can't we just remove the module-level setupWPTimezone at the bottom of the file and mark the package as sideEffects: false ?

I'll also cc @epeicher & @jsnajdr because this seems like it could (should) have been done by virtue of the changes made in #75831.

In fact, if I follow the code, setupWPTimezone should already be called by these functions anyways because of how they use ensureWPTimezone. Can't we just remove the module-level setupWPTimezone at the bottom of the file and mark the package as sideEffects: false ?

I'm totally down with that! I still think it's technically a breaking change, but it's probably fine. If someone imported the library like import '@wordpress/date' the behavior of their app will change.

I still think it's technically a breaking change, but it's probably fine. If someone imported the library like import '@wordpress/date' the behavior of their app will change.

Could you explain a bit more what you'd expect to change? Maybe a practical example of a regression would be helpful. My impression is that the behavior doesn't change at all.

Currently, the library is wp library (wp.date). Meaning other libraries can enqueue by simply having date or wp-date in their dependency arrays. When it's in the dependency array, it will be enqueued automatically. When it's enqueued, it runs setupWPTimezone because it's in the module scope.

So if you have a library that has date in its deps array, the lib guarantees WP Timezone will be set. If I remove the function from the module scope, it won't be.

I'm not 100% confident, but that's is how I understand WP dependencies.

Mmm, okay, I think I understand a bit better now that I'm looking at the code again.

My impression was: It doesn't matter if WP timezone is set until something that uses it (i.e. a function within the package) is called.

But in reality I guess we're working with a shared moment.js module, so defining the timezone is something that can be accessed by anyone working with WordPress's copy of moment.js, even outside the date package. So technically someone could load the date script, then expect to work directly with moment.js in a way that's preconfigured with the WordPress timezone settings.

That's a rather unfortunate setup, and maybe an exceedingly rare use-case, but it is indeed a behavior that existed 😞

Can you help me understand the use-case a bit better? I'm trying to follow whether the goal here is to get access to the "default" settings from how we'd expect the backend to behave (which could motivate a DEFAULT_SETTINGS constant export, for example). Or why someone would want to work with the settings object but not actually any of the other utilities from the date package.

Is there any reason to introduce the "current" verbiage in these APIs, or could we just continue to use getSettings & setSettings as we did already? It seems confusing that I'd use getSettings if I import from @wordpress/date but then I'd use getCurrentSettings instead if I import from @wordpress/date/settings.

Can you help me understand the use-case a bit better? I'm trying to follow whether the goal here is to get access to the "default" settings from how we'd expect the backend to behave (which could motivate a DEFAULT_SETTINGS constant export, for example). Or why someone would want to work with the settings object but not actually any of the other utilities from the date package.

Absolutely. The motivation behind this PR was this. The Number Formatters library in Jetpack uses the wp.data.getSettings call to get the locale data. It does not depend on any of the utilities of the library. In fact, it does not depend on the library at all, but it depends on the global set by its PHP side.

And here's why using that global directly is a bad idea.

Is there any reason to introduce the "current" verbiage in these APIs,

The only reason is I didn't want a clash between the root's getSettings and the one in @wordpress/date/settings if they were to have the same name. I removed that now.

Absolutely. The motivation behind this PR was this. The Number Formatters library in Jetpack uses the wp.data.getSettings call to get the locale data. It does not depend on any of the utilities of the library. In fact, it does not depend on the library at all, but it depends on the global set by its PHP side.

Thanks for the context. It definitely helps. So is the main thing you're interested in is only the user locale from the date settings (source)? I wonder if there'd be a more direct way to get that. It's coming from get_user_locale on the back-end (source). I wonder if document.documentElement.lang (the lang attribute on <html> element) would give the same. An AI-assisted analysis seems to conclude it would, though it's a bit indirect 😅 (via language_attributes). The locale-* body class is a bit more direct, albeit requiring some transformation.

I'm not strictly opposed to more direct access to the date package settings, though in this case it feels like you're generally not interested in the date package except for getting access to various backend settings that we expose through the package, and there may be alternative or more direct ways to get to those settings.

You are right. I don't think this is worth changing the library over. I'll find an alternative.

I think that might be the right choice for your given use-case, but I'll just say for posterity that I'd be willing to leave the door open for something like this in the future. It seems like @wordpress/date settings are the primary way that we expose a lot of back-end date-related settings to JavaScript, and so maybe it could be a good thing to have a way to get access to those settings without pulling in all of the utilities. I think it ends up being a bit complicated because something needs to enqueue wp-date and all its dependencies for those settings to be initialized. Maybe this speaks to a need for a lighter "settings" layer.