Skip to content

Fix c and r formats, add tests. #27002

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.

Sign up for GitHub

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

Merged
vcanales merged 1 commit into from
Nov 16, 2020
Merged

Fix c and r formats, add tests. #27002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix c and r formats, add tests.
  • Loading branch information
Vicente Canales committed Nov 16, 2020
commit 1f737025dd65449707f3e8cde789ac0b9740ff7e
28 changes: 25 additions & 3 deletions packages/date/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import {
isFuture,
isLeapYear,
parseISO,
toDate,
} from 'date-fns';
import {
format as formatTZ,
utcToZonedTime,
zonedTimeToUtc,
toDate,
} from 'date-fns-tz';
import originalLocale from 'date-fns/locale/en-US/index';
import buildLocalizeFn from 'date-fns/locale/_lib/buildLocalizeFn';
Expand Down Expand Up @@ -306,8 +306,30 @@ const formatMap = {
);
},
// Full date/time
c: 'yyyy-MM-DDTHH:mm:ssZ', // .toISOString
r: 'ddd, D MMM yyyy HH:mm:ss ZZ',
c( dateValue ) {
return formatTZ(
utcToZonedTime(
zonedTimeToUtc( dateValue, getActualTimezone() ),
'UTC'
), // Offsets the time to the correct timezone
"yyyy-MM-dd'T'HH:mm:ssXXX",
{
timeZone: getActualTimezone(), // Adds the timezone offset to the Date object that will be formatted.
Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vcanales, we are forcing the output to be in the website timezone. The c format may be used in wp.date.format for example. That function should never change the timezone the date passed to it contains. So the c format should just output things without any manipulations of timezones.
I added unit tests that show the issue at #27272.

We also have a bug in the logic in this function, if we pass wp.date.format a date that is already on the website timezone we receive a date with different hours (wrong output).

I guess c format should just map to "yyyy-MM-dd'T'HH:mm:ssXXX" and it should be up to other functions like dateI18n etc to manipulate the timezones if they need that.

Copy link
Member Author

@vcanales vcanales Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for revisiting this!

I guess c format should just map to "yyyy-MM-dd'T'HH:mm:ssXXX" and it should be up to other functions like dateI18n etc to manipulate the timezones if they need that.

I believe you are correct, but there are some caveats. Some of these additional timezone offsets were necessary because Date objects are interpreted by the browser as local time, unless we explicitly offset them. These roundabout timezone settings are trying to get the Browser to read the time as-is instead of applying the Browser timezone.

All of the above is necessary because WordPress timestamps do not include any timezone information at all, the post's date attribute is in the site's timezone, date_gmt is in GMT, and neither come with a suffix indicating this fact, so it's on @wordpress/date to set the timezone for Date, otherwise the Browser will think that they're all local times. This means that the date formatting functions, if we don't do any timezone offsetting on @wordpress/date, will only work if the site's timezone is the same as the Browser's, otherwise they'll be inaccurate. This was surfaced on at least one issue even before the refactor: #15673

I think this means that we might want to refactor the tests—removing all indication of timezones on the timestamps we test against will actually reflect data we will receive from the API. From there, we have to figure out how to reliably offset timezones on @wordpress/date.

I want to note also that adding timezone information to the timestamps on the API side would help to reduce a lot of the timezone juggling (getting the site's timezone from settings, trying to apply it on Date, offsetting the Browser's timezone if they're not the same, etc) on @wordpress/date.

Copy link
Member Author

@vcanales vcanales Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a code sandbox with a bunch of examples, showing why some of these manual offsets are necessary even if we want to keep the date intact when formatting, explicitly when the browser's timezone is different from the site's:

https://codesandbox.io/s/boring-microservice-d9rpb?file=/src/index.js

Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this means that we might want to refactor the tests—removing all indication of timezones on the timestamps we test against will actually reflect data we will receive from the API. From there, we have to figure out how to reliably offset timezones on @wordpress/date.

Hi @vcanales,

I agree with the tests we have don't cover the use cases of WordPress. But we should not remove all indications of timezones. Our documentation specifies that dates can contain timezones and a plugin developer may pass date's with timezones. We are publishing an npm module that may even be used outside WordPress. So I think all the tests we have are valid and should be kept. We should probably add more tests with dates that don't contain a timezone and probably use a mocking library to simulate local timezones like https://www.npmjs.com/package/timezone-mock.

We currently have:
- format, that should format a date without translations or any timezone manipulation whatsoever.
- dateI18n this function received a date if the date did not contain a timezone, it assumed the date to be in the users timezone, and formated the date to be in the sitetime zone or the timezone argument.
- gmdateI18n manipulates dates in UTC timezone.

In WordPress, the main use case is receiving a date without timezone that we should assume is on the website timezone and format it keeping the website timezone. We don't have any function that does that and that's the reason we had bugs for some time.

In #26276 I'm finding that and adding a function that assumes a date is in WordPress timezone contrary to dateI18n that assumes user, and gmdateI18n that assumes UTC.

}
);
}, // .toISOString
r( dateValue ) {
return formatTZ(
utcToZonedTime(
zonedTimeToUtc( dateValue, getActualTimezone() ),
'UTC'
), // Offsets the time to the correct timezone
'iii, d MMM yyyy HH:mm:ss XX',
{
timeZone: getActualTimezone(), // Adds the timezone offset to the Date object that will be formatted.
}
);
},
U( dateValue ) {
return formatTZ(
zonedTimeToUtc( dateValue, getActualTimezone() ),
Expand Down
21 changes: 17 additions & 4 deletions packages/date/src/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,22 +471,35 @@ describe( 'PHP Format Tokens', () => {
setSettings( settings );
} );

it.skip( 'should support "c" to obtain ISO 8601 date', () => {
it( 'should support "c" to obtain ISO 8601 date', () => {
const settings = __experimentalGetSettings();

setSettings( {
...settings,
timezone: { offset: -2 },
timezone: { offset: -5, string: 'America/Bogota' },
} );

const formattedDate = dateNoI18n( 'c', '2019-06-18T11:00:00.000Z' );

expect( formattedDate ).toBe( '2019-06-18T11:00:00-02:00' );
expect( formattedDate ).toBe( '2019-06-18T11:00:00-05:00' );

setSettings( settings );
} );

it( 'should support "r" RFC 2822 formatted date', () => {} );
it( 'should support "r" RFC 2822 formatted date', () => {
const settings = __experimentalGetSettings();

setSettings( {
...settings,
timezone: { offset: -5, string: 'America/Bogota' },
} );

const formattedDate = dateNoI18n( 'r', '2019-06-18T11:00:00.000Z' );

expect( formattedDate ).toBe( 'Tue, 18 Jun 2019 11:00:00 -0500' );

setSettings( settings );
} );

it( 'should support "U" to get epoc for given date', () => {
const settings = __experimentalGetSettings();
Expand Down