Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dateI18n throws Moment error with String timezones #22193

Closed
iandunn opened this issue May 7, 2020 · 3 comments · Fixed by #22866
Closed

dateI18n throws Moment error with String timezones #22193

iandunn opened this issue May 7, 2020 · 3 comments · Fixed by #22866
Assignees
Labels
[Package] Date /packages/date [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@iandunn
Copy link
Member

iandunn commented May 7, 2020

Problem

Calling dateI18n -- and possibly other date functions -- produces a console error, if the timezone is set to a string like America/Vancouver. If it's set to an int like, -7 then it doesn't throw an error.

Moment Timezone has no data for America/Vancouver. See http://momentjs.com/timezone/docs/#/data-loading/. index.js:6:3194

The times appear correct regardless of whether the error is shown, but I haven't tested edge cases like DST, etc.

Possibly related: #21977, #18982

Reproduction steps

Tested with Firefox and the master branch for Core and the plugin.

  1. Set Core's timezone to America/Vancouver.
  2. Add the following to an mu-plugin:
add_action( 'admin_enqueue_scripts', function() {
	wp_enqueue_script( 'wp-date' );

	wp_add_inline_script(
		'wp-date',
		'
			console.log(
				wp.date.dateI18n( "Y-m-d h:ia", Date.now() ) 
			);
		',
		'after'
	);
} );
  1. Open the console and open an admin page, the error should appear.
  2. If you set the timezone to -7 then it will work without an error.

In a React app, this results in an error:

import { dateI18n } from '@wordpress/date';
dateI18n( 'Y-m-d', session.derived.startTime );

...while this doesn't:

import { dateI18n, setSettings, __experimentalGetSettings } from '@wordpress/date';
import 'moment-timezone/moment-timezone-utils';
setSettings( __experimentalGetSettings() );
dateI18n( 'Y-m-d', session.derived.startTime );
@iandunn iandunn added [Package] Date /packages/date [Type] Bug An existing feature does not function as intended labels May 7, 2020
@ryelle
Copy link
Contributor

ryelle commented May 18, 2020

The issue is that moment-timezone isn't loading any of the timezone data. It seems like this was intentional, from #12356, because technically WP uses its own timezone WP. (You can see what zones are available with moment.tz.names().)

First I thought maybe we could always rely on the int offset, and remove these lines. That does fix the example you've given, but it's also possible to pass in a custom timezone string to dateI18n, and then the same error pops up.

wp.date.dateI18n( 'Y-m-d h:ia', Date.now(), 'Asia/Macau' );

// Console: Moment Timezone has no data for Asia/Macau.

I think either we need to bring back the timezone data, or disallow string timezones.

As an aside, I'm not totally sure why import 'moment-timezone/moment-timezone-utils'; works here, it does end up importing all of moment-timezone + data, while in packages/date/src/index.js, it only imports the timezone utils. The same thing seems to happen in the tests, because all timezones are available there (which is why the tests pass).

@davilera
Copy link
Contributor

davilera commented Jun 5, 2020

The times appear correct regardless of whether the error is shown, but I haven't tested edge cases like DST, etc.

I'm guessing they're "correct" because the timezone of your WordPress and the timezone you’re computer is in are the same.

The issue is that moment-timezone isn't loading any of the timezone data. It seems like this was intentional, from #12356, because technically WP uses its own timezone WP. (You can see what zones are available with moment.tz.names().)

I've tested @iandunn's issue on WordPress 5.4.1 (that is, I didn't exactly reproduce his steps) and this is working as expected:

moment.tz.names().includes( 'America/Vancouver' )
» true

So I really don't know which timezones are available and which aren't. Maybe someone from #12356 can shed some light on this one?

I think either we need to bring back the timezone data, or disallow string timezones.

I agree. In #18982 we decided to include string timezones so that JS API resembled that of PHP (namely, wp_date), so I think it'd be a good idea to bring back timezone data.

@ryelle
Copy link
Contributor

ryelle commented Jun 5, 2020

WordPress 5.4.1 […] is working as expected

Yeah, this is only an issue in the plugin, because 5.4.1 is built with a later version of moment-timezone. The later versions (0.5.18+, i think) load all the timezone data due to a change in how they import files. That's also why importing moment-timezone/moment-timezone-utils locally into a project can fix this, because it pulls in a higher version.

I've updated moment-timezone in #22866, which brings back the timezone data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Date /packages/date [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants