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

Date package: Fix timezone issues #27519

Closed
wants to merge 9 commits into from

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Dec 4, 2020

Fixes #27500 #27251

Description

Removed getDate and isInTheFuture in favor of isSiteDateInTheFuture, parseSiteData, siteDateToLocalDate and localDateToSiteDate. These functions are supposed to be more descriptive to avoid confusion and bugs.

How has this been tested?

You can follow testing steps in the two mentioned issues: #27500 #27251

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Dec 4, 2020

Size Change: +525 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/editor-rtl.css 9.07 kB -3 B (0%)
build/block-library/editor.css 9.07 kB -3 B (0%)
build/block-library/index.js 149 kB +26 B (0%)
build/date/index.js 11.3 kB +129 B (1%)
build/edit-post/index.js 306 kB +2 B (0%)
build/edit-site/index.js 24.5 kB +359 B (1%)
build/edit-widgets/index.js 26.3 kB +1 B
build/editor/index.js 43.6 kB +14 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.2 kB 0 B
build/block-editor/style.css 11.2 kB 0 B
build/block-library/style-rtl.css 8.34 kB 0 B
build/block-library/style.css 8.34 kB 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 15.2 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.98 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/style-rtl.css 6.49 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/style-rtl.css 3.91 kB 0 B
build/edit-site/style.css 3.91 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.63 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.74 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.93 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.4 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@ockham
Copy link
Contributor

ockham commented Dec 4, 2020

👋 @david-szabo97 I think that @vcanales is working on a fix in #27398, ICMYI 😊

@vcanales
Copy link
Member

vcanales commented Dec 4, 2020

Hi, thanks for taking on this. I believe our approaches to fix the assumptions that were made before are similar. We also tackled some of the issues of usage I was facing regarding the scheduled post selector. I'm not sure how to proceed though; tests are mostly passing on #27398, and there’s a few eyes on it already. Let me know what you think!

@david-szabo97
Copy link
Member Author

@vcanales I'm happy to review and help to debug yours! Thank you for your work!

I'm a bit unhappy with the current public API of the date package. I think getting rid of getDate and replacing it with a few "timezone" converter functions is more descriptive and easier to use. Please take a look at my implementation and see if it makes sense to you. Although I couldn't perfect the implementation and tests are still failing, I think a new public API would make sense.

@vcanales
Copy link
Member

vcanales commented Dec 4, 2020

I'm a bit unhappy with the current public API of the date package. I think getting rid of getDate and replacing it with a few "timezone" converter functions is more descriptive and easier to use.

I think this makes sense, although removing those functions would also make it a breaking change; I even found that I broke some things while trying to not alter the API (which is also kind of why the current implementation is not the cleanest). The current API is used by some plugin makers for instance, so a transition period might be necessary.

I do agree that publishing time zone utilities would be preferable, and would also be more in tune of the functional approach of date-fns.

Perhaps we could consider making 2 steps out of this: getting the current breakage out of the way, and then rethink @wordpress/date in a way in which it's less influenced by the previous momentjs-based implementation, applying what you've done here.

@gziolo gziolo added the [Package] Date /packages/date label Dec 4, 2020
@gziolo
Copy link
Member

gziolo commented Dec 4, 2020

The current API is used by some plugin makers for instance, so a transition period might be necessary.

There shouldn't be such thing like a transition period in terms of API exposed in the WordPress Core. The expectation is to keep old method signatures even when deprecated forever.

@david-szabo97
Copy link
Member Author

We can keep the old functions and deprecate them. Meanwhile, we can introduce new functions and use them in Gutenberg. This can be done in one or two steps.

@ockham
Copy link
Contributor

ockham commented Dec 7, 2020

Per convo with @david-szabo97, we'll focus on getting #27398 ready for now in order to avoid removing functions.

@gziolo gziolo deleted the fix/date-package-timezone-issues branch December 7, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Date /packages/date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date: Tests are false-positive, assumes it is ran in GMT timezone and en_US locale
4 participants