date-fns-tz icon indicating copy to clipboard operation
date-fns-tz copied to clipboard

Update date-fns to v3

Open christopherklint97 opened this issue 2 years ago • 26 comments

Summary

This PR updates date-fns to v3 and adds support for it. Some other minor changes include:

  • Adding @babel/preset-env and removing the individual babel plugin transforms since they are included.
  • Running xit on the tests that expected a RangeError. This error does not seem to occur anymore in date-fns v3 but have chosen not to remove it in case these tests want to be rewritten.

christopherklint97 avatar Dec 28 '23 07:12 christopherklint97

We do not miss a loader, but we need to support https://babeljs.io/docs/babel-plugin-transform-logical-assignment-operators.

I understand that there are two options

  • explicitly use the plugin
  • use preset-env

Plugin

{
  "plugins": ["@babel/plugin-transform-logical-assignment-operators"]
}

@babel/preset-env

  "presets": [
    ["@babel/env", {
      "targets": ["2021"], // at least 2021
    }]

Here is a checklist for other changes we might need:

  • [ ] update peerDependencies to >=3.0.0 for date-fns (related to https://github.com/marnusw/date-fns-tz/pull/262)
  • [ ] Should .yarn/releases be committed? https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored

antoinepairet avatar Dec 28 '23 14:12 antoinepairet

Thanks everyone, I hope to find time for this soon.

marnusw avatar Dec 28 '23 18:12 marnusw

Thanks for the reviews @antoinepairet @neilhem @JeromeFitz ! 🙌🏾

I got the tests to run now. Looks like 6 tests are failing at the moment, most are missing RangeErrors and some incorrect timezones.

$ yarn test

START:
ℹ 「wdm」: Built at: 12/29/2023 9:46:54 AM
ℹ 「wdm」: Compiled successfully.
ℹ 「wdm」: Compiling...
⚠ 「wdm」: Built at: 12/29/2023 9:46:55 AM
Entrypoint ../test = ../test.js
[./node_modules/date-fns/format.js] 25.7 KiB {../test} [built]
[./node_modules/date-fns/locale/en-GB.js] 772 bytes {../test} [built]
[./node_modules/power-assert/index.js] 2.2 KiB {../test} [built]
[./src sync recursive \/test\.js$] ./src sync \/test\.js$ 460 bytes {../test} [built]
[./src/_lib/newDateUTC/test.js] 36.4 KiB {../test} [optional] [built]
[./src/_lib/tzIntlTimeZoneName/test.js] 22.4 KiB {../test} [optional] [built]
[./src/_lib/tzParseTimezone/test.js] 127 KiB {../test} [optional] [built]
[./src/_lib/tzTokenizeDate/test.js] 31.8 KiB {../test} [optional] [built]
[./src/format/test.js] 171 KiB {../test} [optional] [built]
[./src/formatInTimeZone/test.js] 16.4 KiB {../test} [optional] [built]
[./src/getTimezoneOffset/test.js] 100 KiB {../test} [optional] [built]
[./src/toDate/test.js] 255 KiB {../test} [optional] [built]
[./src/utcToZonedTime/test.js] 53.5 KiB {../test} [optional] [built]
[./src/zonedTimeToUtc/test.js] 89.8 KiB {../test} [optional] [built]
[./test.js] 124 bytes {../test} [built]
    + 241 hidden modules

WARNING in ./node_modules/power-assert-formatter/lib/create.js 30:28-49
Critical dependency: the request of a dependency is an expression
 @ ./node_modules/power-assert-formatter/index.js
 @ ./node_modules/power-assert/index.js
 @ ./src/format/test.js
 @ ./src sync \/test\.js$
 @ ./test.js
ℹ 「wdm」: Compiled with warnings.
29 12 2023 09:46:55.911:INFO [karma-server]: Karma v6.4.2 server started at http://localhost:9876/
29 12 2023 09:46:55.912:INFO [launcher]: Launching browsers LocalChrome with concurrency unlimited
29 12 2023 09:46:55.925:INFO [launcher]: Starting browser Chrome
29 12 2023 09:46:56.960:INFO [Chrome 120.0.0.0 (Mac OS 10.15.7)]: Connected on socket f5AV3NtniDRTrLKXAAAB with id 79769157

Finished in 0.004 secs / 0.023 secs @ 09:46:57 GMT+0100 (GMT+01:00)

SUMMARY:
✔ 259 tests completed
ℹ 1 test skipped
✖ 6 tests failed

FAILED TESTS:
  format
    ✖ throws `RangeError` if `options.weekStartsOn` is not convertable to 0, 1, ..., 6 or undefined
      Chrome 120.0.0.0 (Mac OS 10.15.7)
    AssertionError: Missing expected exception (RangeError)..
        at _throws (webpack:///node_modules/assert/assert.js:454:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1163:5)
        at ./node_modules/assert/assert.js.assert.throws (webpack:///node_modules/assert/assert.js:478:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1187:3)
        at Context.<anonymous> (webpack:///src/format/test.js:865:12 <- /Users/christopherklint/coding/date-fns-tz/test.js:29176:26)

    ✖ throws `RangeError` if `options.firstWeekContainsDate` is not convertable to 1, 2, ..., 7 or undefined
      Chrome 120.0.0.0 (Mac OS 10.15.7)
    AssertionError: Missing expected exception (RangeError)..
        at _throws (webpack:///node_modules/assert/assert.js:454:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1163:5)
        at ./node_modules/assert/assert.js.assert.throws (webpack:///node_modules/assert/assert.js:478:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1187:3)
        at Context.<anonymous> (webpack:///src/format/test.js:873:12 <- /Users/christopherklint/coding/date-fns-tz/test.js:29183:26)

    time zone
      ✖ Local date with specific non-location time zone
        Chrome 120.0.0.0 (Mac OS 10.15.7)
      AssertionError:   # src/format/test.js:669

        assert(result === dateAndTimeZoneAmericaNY)
               |      |   |
               |      |   "1986-04-04 10:32:55 EST"
               |      false
               "1986-04-04 10:32:55 GMT-5"

        --- [string] dateAndTimeZoneAmericaNY
        +++ [string] result
        @@ -17,7 +17,9 @@
         :55
        -EST
        +GMT-5


          at ./node_modules/empower-core/lib/decorator.js.Decorator._callFunc (webpack:///node_modules/empower-core/lib/decorator.js:110:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12223:20)
          at ./node_modules/empower-core/lib/decorator.js.Decorator.concreteAssert (webpack:///node_modules/empower-core/lib/decorator.js:103:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12216:17)
          at decoratedAssert (webpack:///node_modules/empower-core/lib/decorate.js:49:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12084:30)
          at powerAssert (webpack:///node_modules/empower-core/index.js:63:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12007:32)
          at Context.<anonymous> (webpack:///src/format/test.js:669:13 <- /Users/christopherklint/coding/date-fns-tz/test.js:28804:32)

      ✖ https://github.com/marnusw/date-fns-tz/issues/138
        Chrome 120.0.0.0 (Mac OS 10.15.7)
      AssertionError:   # src/format/test.js:685

        assert.equal(result, '2020-10-31 9:37pm -05:00 -05:00 GMT-5 CDT')
                     |
                     "2020-10-31 9:37pm -05:00 -05:00 GMT-5 GMT-5"

          at ./node_modules/empower-core/lib/decorator.js.Decorator._callFunc (webpack:///node_modules/empower-core/lib/decorator.js:110:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12223:20)
          at ./node_modules/empower-core/lib/decorator.js.Decorator.concreteAssert (webpack:///node_modules/empower-core/lib/decorator.js:103:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12216:17)
          at Function.decoratedAssert [as equal] (webpack:///node_modules/empower-core/lib/decorate.js:49:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:12084:30)
          at Context.<anonymous> (webpack:///src/format/test.js:685:14 <- /Users/christopherklint/coding/date-fns-tz/test.js:28837:28)

    custom locale
      ✖ throws `RangeError` if `options.locale` doesn't have `localize` property
        Chrome 120.0.0.0 (Mac OS 10.15.7)
      AssertionError: Missing expected exception (RangeError)..
          at _throws (webpack:///node_modules/assert/assert.js:454:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1163:5)
          at ./node_modules/assert/assert.js.assert.throws (webpack:///node_modules/assert/assert.js:478:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1187:3)
          at Context.<anonymous> (webpack:///src/format/test.js:844:14 <- /Users/christopherklint/coding/date-fns-tz/test.js:29157:28)

      ✖ throws `RangeError` if `options.locale` doesn't have `formatLong` property
        Chrome 120.0.0.0 (Mac OS 10.15.7)
      AssertionError: Missing expected exception (RangeError)..
          at _throws (webpack:///node_modules/assert/assert.js:454:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1163:5)
          at ./node_modules/assert/assert.js.assert.throws (webpack:///node_modules/assert/assert.js:478:1 <- /Users/christopherklint/coding/date-fns-tz/test.js:1187:3)
          at Context.<anonymous> (webpack:///src/format/test.js:856:14 <- /Users/christopherklint/coding/date-fns-tz/test.js:29168:28)

@marnusw you probably know best here if we should keep the RangError checks and implement them in date-fns-tz or just remove them?

christopherklint97 avatar Dec 29 '23 09:12 christopherklint97

Thanks for making the time on the latest and greatest. Awesome, work @christopherklint97 !

For the RangeError ones date-fns@3 introduced types to inform the developer that they shouldn't do these things. So that may mean that intentionally breaking in a test will no longer work, since the errors themselves have changed. (I ran into these exact ones in an out-of-scope move to types.)

But would defer to someone who knows more on this package and karma for sure.

For this one though:

throws RangeError if options.firstWeekContainsDate is not convertable to 1, 2, ..., 7 or undefined

This changed to only allow 1 | 4 in date-fns@3 (in 2021):

  • Reference: https://github.com/date-fns/date-fns/blob/main/src/types.ts#L138

I believe this should also happen in date-fns-tz given the comments in date-fns code (reference in #2427). If so this would need to be changed in a few references:

  • https://github.com/marnusw/date-fns-tz/blob/master/typings.d.ts#L32
  • https://github.com/marnusw/date-fns-tz/blob/master/docs/OptionsWithTZ.js#L13

Every index.js.flow would get their update in the next build per the current structure.

Though now that date-fns@3 exports types this could be changed further in typings.d.ts:

  • Some other slight changes with weekStartsOn|unit|roundingMethod should be confirmed if date-fns-tz has different values or should be kept in-sync.
import type { Day, FirstWeekContainsDate, LocaleUnit, RoundingMethod } from "date-fns";
import type { Locale } from 'date-fns'

type OptionsWithTZ = {
  weekStartsOn?: Day,
  firstWeekContainsDate?: FirstWeekContainsDate,
  additionalDigits?: 0 | 1 | 2,
  timeZone?: string,
  locale?: Locale,
  includeSeconds?: boolean,
  addSuffix?: boolean,
  unit?: LocaleUnit,
  roundingMethod?: RoundingMethod,
  awareOfUnicodeTokens?: boolean,
}

declare module.exports: (
  date: Date | string | number,
  timeZone: string,
  formatStr: string,
  options?: OptionsWithTZ
) => string

This is looking real good! Props again! 🎉


Additional Edit:

The Local date with specific non-location time zone error looks to be local on your machine. None of the Ci/CD have it, nor do I. (Who happens to be dateAndTimeZoneAmericaNY.)

Wonder if running:

env TZ=utc yarn run test

Would work locally?

But it is looking just like the RangeErrors:

SUMMARY:
✔ 261 tests completed
ℹ 1 test skipped
✖ 4 tests failed

A potential option if the tests should be kept in until date-fns-tz moves to types or something, their functions could be changed to xit which will tell karma to ignore the test.

JeromeFitz avatar Dec 29 '23 15:12 JeromeFitz

Aye ... digging into this a bit more if the move to date-fns@3 types is to accompany.

The three places of change are:

1️⃣

./scripts/build/_lib/typings/formatBlock.js

L114:

"import type { Day, FirstWeekContainsDate, Locale, LocaleUnit, RoundingMethod } from 'date-fns'\n" +

2️⃣

./scripts/build/_lib/typings/typeScript.js

L296:

      import type { Day, FirstWeekContainsDate, Locale, LocaleUnit, RoundingMethod } from "date-fns"

3️⃣

./docs/OptionsWithTZ.js

Then editing the way they are currently formatted with flow

L9:

 * @property {Day} [weekStartsOn=0] - the index of the first day of the week (0 - Sunday).

L13:

 * @property {FirstWeekContainsDate} [firstWeekContainsDate=1] - the day of January,

L29:

 * @property {LocaleUnit} [unit] - used by `formatDistanceStrict`.

L31:

 * @property {RoundingMethod} [roundingMethod='floor'] - used by `formatDistanceStrict`.

Then after a yarn run build that will update the rest of the files accordingly. 😅

JeromeFitz avatar Dec 29 '23 21:12 JeromeFitz

@marnusw all tests are passing now since I am xiting the RangeError tests. All you have to do is merge and release a new version if you are happy 😉

@JeromeFitz I really appreciate the comments here and making it so easy for me to copy and paste the changes 👍🏾

christopherklint97 avatar Jan 08 '24 08:01 christopherklint97

Looking forward to this 😃

gehringf avatar Jan 08 '24 10:01 gehringf

Hello, what is the status of this PR? Thanks

tomalaforge avatar Jan 12 '24 13:01 tomalaforge

Hello, what is the status of this PR? Thanks

@tomalaforge I would say status here is awaiting a review and possible merge from @marnusw if he is happy with the changes.

christopherklint97 avatar Jan 14 '24 18:01 christopherklint97

@christopherklint97, thanks for this PR! I hope @marnusw merges soon.

bosticka avatar Jan 19 '24 13:01 bosticka

please merge 👯

joacub avatar Jan 22 '24 15:01 joacub

When merge? 👁️ 👄 👁️

Minho-Lee avatar Jan 26 '24 20:01 Minho-Lee

Hi all, thank you very much for all this work! I will do my best to get to this and merge it within a week or so.

marnusw avatar Jan 27 '24 16:01 marnusw

Hi all, thank you very much for all this work! I will do my best to get to this and merge it within a week or so.

Any news @marnusw ?

For people looking at a solution now: https://www.npmjs.com/package/date-fns-tz-v3

keesvanbemmel avatar Feb 07 '24 20:02 keesvanbemmel

Hi all, thank you very much for all this work! I will do my best to get to this and merge it within a week or so.

Any news @marnusw ?

For people looking at a solution now: https://www.npmjs.com/package/date-fns-tz-v3

@marnusw I don't understand why this linked package exists. Should we use that or wait for this PR?

gehringf avatar Feb 14 '24 06:02 gehringf

Hi all, thank you very much for all this work! I will do my best to get to this and merge it within a week or so.

Any news @marnusw ? For people looking at a solution now: https://www.npmjs.com/package/date-fns-tz-v3

@marnusw I don't understand why this linked package exists. Should we use that or wait for this PR?

No, sir. This library should not be used. The following repo will tell you all you need to know

https://github.com/davidramosweb/date-fns-tz

L4Ph avatar Feb 15 '24 08:02 L4Ph

No, sir. This library should not be used. The following repo will tell you all you need to know

https://github.com/davidramosweb/date-fns-tz

Thanks for pointing it out. The npm page for it is super misleading and makes you think @marnusw released it. image

gehringf avatar Feb 15 '24 11:02 gehringf

No, sir. This library should not be used. The following repo will tell you all you need to know https://github.com/davidramosweb/date-fns-tz

Thanks for pointing it out. The npm page for it is super misleading and makes you think @marnusw released it. image

Misleading indeed! had me fooled :(

keesvanbemmel avatar Feb 15 '24 11:02 keesvanbemmel

Does anyone have a working solution other than waiting for this PR to be merged? I don't want to wait any longer.

keesvanbemmel avatar Feb 21 '24 20:02 keesvanbemmel

@keesvanbemmel if you really can't wait you can npm install from any github repo or branch, including the branch for this PR (christopherklint97:update-to-v3).

stephenhmarsh avatar Feb 21 '24 20:02 stephenhmarsh

@keesvanbemmel if you really can't wait you can npm install from any github repo or branch, including the branch for this PR (christopherklint97:update-to-v3).

Thanks will do!

keesvanbemmel avatar Feb 21 '24 20:02 keesvanbemmel

Can we have a release please? Installing from branch may be okay for development but launching a product to production with a dependency like that is not safe. I cannot start using this library because our project was started with date-fns v3 and downgrading is more effort than what it's worth. I get that maintainers may be busy people, but someone already put time and effort into making this PR while community keeps begging for this to be released.

BlindDespair avatar Feb 25 '24 13:02 BlindDespair

I know that maintaining a OSS is a lot of work. I'm maintaining one myself. But here, everything has been done. I don't understand what is stopping you from merging and publishing a new version. It should take 30min max. 🙏

tomalaforge avatar Feb 25 '24 14:02 tomalaforge

In defense of @marnusw this is a new major version with breaking changes and maybe he needs time to consider the implications and how to document. Besides the imports, several tests were turned off because they no longer throw expected errors. If you think installing from a branch is "not safe," publishing a major version without fully understanding the breaking changes is worse. There's literally a comment right above @BlindDespair with a bug.

You can always clone the PR branch to your own repo and lock your install to the exact commit SHA. That's really as safe as you can you can ask for, especially if you're so personally sure this code is 100% ready and @marnusw has no reason not to merge it.

stephenhmarsh avatar Feb 25 '24 21:02 stephenhmarsh

@stephenhmarsh that is so, but this package is linked by date-fns' documentation as go-to solution for working with timezones when using date-fns. Normally other libraries or frameworks update together around the same time, meaning date-fns-tz would take care of these changes before v3 was even released on the main package. Otherwise the date-fns package shouldn't link to this package and not claim that there's a way to work with timezones and maybe explicitly state that their package is not recommended if timezones are necessary.

BlindDespair avatar Feb 25 '24 22:02 BlindDespair

"Patience is a virtue". Does v2 have any security implications, not yet. Is this being worked on for free, yes.

SheldonWBM avatar Feb 26 '24 15:02 SheldonWBM

Please merge @marnusw 🙏

joserodrigorojo avatar Mar 04 '24 23:03 joserodrigorojo

Please merge @marnusw 🙇‍♂️

lobaak avatar Mar 06 '24 23:03 lobaak

@marnusw anything pending for this PR to be merged? If possible release a beta

veeramarni avatar Mar 07 '24 22:03 veeramarni

@marnusw anything pending for this PR to be merged? If possible release a beta

A beta can be official or, use this branch directly. See https://martinwolf.org/2018/04/github-branch-as-dependency-package-json/ Then you can help troubleshoot but, I would not use it for production.

SheldonWBM avatar Mar 07 '24 22:03 SheldonWBM