[Feedback Needed] Upgrade to use date-fns-v2 #150

pivotal-james-zcheng · 2020-01-13T13:53:03Z

What kind of change does this PR introduce?
This PR upgrades the date-fns used in the library to V2.9.0. This PR also removes the forked/copied version of date-fns V2.0.0-beta3

Resolves #48

Note
Date-fns-upgrade tool was used to upgrade.

This PR currently compiles and all tests pass. Since the automated tool was used, ~~there are many usages of legacyParse and convertToken in the codebase~~there is one usage of convertToken in the codebase.

This PR is not ready to be merged yet. The PR is created for the changes to go through CI and be available for feedback sooner.

Checklist:

~~- [ ] Documentation~~ NA
~~- [ ] Remove convertToken~~ Not removing to avoid introducing breaking changes

Tests
Remove legacyParse
Ready to be merged

vercel · 2020-01-13T13:53:09Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/arthurdenner/react-semantic-ui-datepickers/iouutrftis
✅ Preview: In Progress

pivotal-james-zcheng · 2020-01-13T14:09:50Z

Date-fns' parse function used to be able to parse dates, numbers and strings. In v2, parse only accepts dates and numbers as arguments. This decision is made to reduce the build size of the library - see blog post.

Many instances of the legacyParse in the codebase are due to the above change. The calendar component accepts minDate and maxDate as DateFns which is defined as Date | string | number. Range prop hoveredDate is a DateFns as well. legacyParse are mostly used for these DateFns.

Can some if not all of these be safely changed to Date without introducing breaking changes?

arthurdenner

Thank you very much, @pivotal-james-zcheng!
This is a long-wanted change and I didn't know this upgrade package, very handy!

I've pointed some minor changes as I just ran through the code here on GitHub.

I kindly ask you to give me some days to try and test these changes. Our test coverage is not so good at the moment - but it's increasing. Maybe we can try to add more tests before moving with these changes? I'll try to do it, but feel free to do it as well if you want to - just let me know.

I'm about to release another project too and that's another reason to hold this PR for some days as I'm completely focused on it.

Thank you once again for your help.
I'll come back with more info as soon as I can!

arthurdenner · 2020-01-13T14:28:25Z

package.json

@@ -34,9 +33,10 @@
  "license": "MIT",
  "dependencies": {
    "@babel/runtime": "7.7.7",
+    "@date-fns/upgrade": "1.0.1",


I don't think this package needs to be installed in the project as we can run it with npx. But if you wish to keep it for convenience, can you move it to be a devDependency, please?

The codemode tool (date-fns-upgrade-codemod) can be run using npx which does not require us to add additional dependencies. Unfortunately, transitional methods provided by support package date-fnd-upgrad, such as legacyParse and convertToken , require us to add the package as a main dependency.

I have worked to remove the usages for legacyParse in this PR, but I am leaning towards keeping convertToken.

In v2.0, date-fns introduced breaking changes to which format strings they accept. Some changes include accepting yyyy instead of YYYY, and dd instead of DD. In addition, characters are now escaped using single quote symbols (') instead of square brackets. See corresponding section in the changelog

I have not been able to find a way to remove convertToken and still pass the tests..

Considering the new format string validations are more strict than before, and the format is different, I am inclined to keep convertToken to avoid introducing breaking changes for anyone using this library.

src/__tests__/utils.test.ts

src/index.tsx

src/pickers/range.tsx

src/utils.ts

pivotal-james-zcheng · 2020-01-13T15:09:29Z

I've decided to go ahead with removing DateFns and switching all props to Date. I am going with this approach because the minDate and maxDate props of calendar component are Dayzed props which are Dates by default.

I am not familiar with the range prop hoveredDate but the tests seem to pass. Please review and let me know.

pivotal-james-zcheng · 2020-01-13T15:49:32Z

@arthurdenner Please review this PR and let me know if any of the changes are breaking functionalities. The tests seem to pass and the widget works well in the storybook, but I am not sure if I missed anything not covered by tests.

Thanks!

- everything compiles and tests pass - using legacyParse Todo - remove forked version of date fns - remote legacyParse if possible

- Split parseOnBlur into single and range methods to fix "Property 'every' does not exist on type 'Date | Date[]'" error

codecov · 2020-02-02T18:31:05Z

Codecov Report

Merging #150 into develop will increase coverage by 0.05%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##           develop     #150      +/-   ##
===========================================
+ Coverage    88.13%   88.19%   +0.05%     
===========================================
  Files           14       14              
  Lines          430      432       +2     
  Branches        79       78       -1     
===========================================
+ Hits           379      381       +2     
  Misses          49       49              
  Partials         2        2

Impacted Files	Coverage Δ
src/components/calendar/calendar.tsx	`100% <ø> (ø)`	⬆️
src/index.tsx	`93.16% <100%> (+0.04%)`	⬆️
src/pickers/range.tsx	`78.33% <100%> (ø)`	⬆️
src/utils.ts	`97.82% <93.75%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e48cb2c...19dfb8d. Read the comment docs.

arthurdenner · 2020-02-02T18:57:08Z

@pivotal-james-zcheng, sorry for the wait to update this PR, I was busy with another project.

I've added more tests on #170 and rebased this PR to get the latest changes and solve conflicts. Now I'm more confident to merge this change. 😃

Thank you very much once again! 🎉

arthurdenner · 2020-03-25T17:53:47Z

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

vercel bot deployed to Preview January 13, 2020 13:53 View deployment

pivotal-james-zcheng force-pushed the date-fns-v2 branch from 5ce52f1 to c293eaf Compare January 13, 2020 14:29

vercel bot deployed to Preview January 13, 2020 14:29 View deployment

arthurdenner reviewed Jan 13, 2020

View reviewed changes

vercel bot deployed to Preview January 13, 2020 15:05 View deployment

vercel bot deployed to Preview January 13, 2020 15:11 View deployment

pivotal-james-zcheng changed the title ~~[WIP and Feedback Needed] Upgrade to use date-fns-v2~~ [Feedback Needed] Upgrade to use date-fns-v2 Jan 13, 2020

arthurdenner force-pushed the date-fns-v2 branch from 021984f to 5cd4afc Compare February 2, 2020 18:25

vercel bot temporarily deployed to Preview February 2, 2020 18:25 Inactive

pivotal-james-zcheng added 6 commits February 2, 2020 19:27

Ignore .idea folder

e987077

Update date-fns to v2 using upgrade helpers

f5f2ca4

- everything compiles and tests pass - using legacyParse Todo - remove forked version of date fns - remote legacyParse if possible

Remove forked date-fns-v2

b156bf5

- Split parseOnBlur into single and range methods to fix "Property 'every' does not exist on type 'Date | Date[]'" error

Remove unnecessary legacyParse of today in test

1aaf02c

Change DateFns to Date and remove usages of legacyParse

9a7dadf

Rearranged imports in util test class

19dfb8d

arthurdenner force-pushed the date-fns-v2 branch from 5cd4afc to 19dfb8d Compare February 2, 2020 18:28

vercel bot deployed to Preview February 2, 2020 18:28 View deployment

arthurdenner merged commit 35ef487 into arthurdenner:develop Feb 2, 2020

arthurdenner added the released label Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feedback Needed] Upgrade to use date-fns-v2 #150

[Feedback Needed] Upgrade to use date-fns-v2 #150

pivotal-james-zcheng commented Jan 13, 2020 •

edited by arthurdenner

Loading

vercel bot commented Jan 13, 2020 •

edited

Loading

pivotal-james-zcheng commented Jan 13, 2020

arthurdenner left a comment

arthurdenner Jan 13, 2020

pivotal-james-zcheng Jan 13, 2020 •

edited

Loading

pivotal-james-zcheng commented Jan 13, 2020

pivotal-james-zcheng commented Jan 13, 2020

codecov bot commented Feb 2, 2020 •

edited

Loading

arthurdenner commented Feb 2, 2020

arthurdenner commented Mar 25, 2020

[Feedback Needed] Upgrade to use date-fns-v2 #150

[Feedback Needed] Upgrade to use date-fns-v2 #150

Conversation

pivotal-james-zcheng commented Jan 13, 2020 • edited by arthurdenner Loading

vercel bot commented Jan 13, 2020 • edited Loading

pivotal-james-zcheng commented Jan 13, 2020

arthurdenner left a comment

Choose a reason for hiding this comment

arthurdenner Jan 13, 2020

Choose a reason for hiding this comment

pivotal-james-zcheng Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

pivotal-james-zcheng commented Jan 13, 2020

pivotal-james-zcheng commented Jan 13, 2020

codecov bot commented Feb 2, 2020 • edited Loading

Codecov Report

arthurdenner commented Feb 2, 2020

arthurdenner commented Mar 25, 2020

pivotal-james-zcheng commented Jan 13, 2020 •

edited by arthurdenner

Loading

vercel bot commented Jan 13, 2020 •

edited

Loading

pivotal-james-zcheng Jan 13, 2020 •

edited

Loading

codecov bot commented Feb 2, 2020 •

edited

Loading