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

Replace remaining moment-js calls with date-fns #13728

Merged
merged 3 commits into from Jul 14, 2022
Merged

Conversation

gu-stav
Copy link
Contributor

@gu-stav gu-stav commented Jul 8, 2022

What does it do?

#13579 made me curious why we do have moment-js next to date-fns in our codebase. Turns out: only because of one call that can easily be replaced by date-fns. This should remove approx. 290kb / 70kb (gzipped) from the frontend bundle and makes it clear that we are using date-fns primarily for handle dates.

Before After
Screenshot 2022-07-08 at 13 36 45 Screenshot 2022-07-08 at 13 37 00

I think it is safe to remove the check and corresponding test for removeKeyInObject: the function is only called by the EditViewDataManager, which (at least since v4) should not pass around moment-js dates anymore.

Why is it needed?

To keep the dependencies clean (moment-js still needs to be installed as sub-dependency though) and keep the bundle size small.

How to test it?

  1. Test creating a new role (using the EE) - the prefilled date in the description should match the current date

@gu-stav gu-stav added source: core:admin Source is core/admin package pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels Jul 8, 2022
@gu-stav gu-stav added this to the 4.2.3 milestone Jul 8, 2022
@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #13728 (7bf5c0d) into master (7dec7f1) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 7bf5c0d differs from pull request most recent head 562c73a. Consider uploading reports for the commit 562c73a to get more accurate results

@@            Coverage Diff             @@
##           master   #13728      +/-   ##
==========================================
- Coverage   54.33%   54.32%   -0.01%     
==========================================
  Files        1197     1197              
  Lines       30596    30594       -2     
  Branches     5563     5562       -1     
==========================================
- Hits        16623    16621       -2     
  Misses      12164    12164              
  Partials     1809     1809              
Flag Coverage Δ
front 56.70% <ø> (-0.01%) ⬇️
unit 48.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nts/EditViewDataManagerProvider/utils/cleanData.js 94.44% <ø> (ø)
...min/src/content-manager/utils/removeKeyInObject.js 95.23% <ø> (-0.42%) ⬇️
...dmin/admin/src/translations/languageNativeNames.js 100.00% <ø> (ø)

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 7dec7f1...562c73a. Read the comment docs.

@alexandrebodin alexandrebodin modified the milestones: 4.2.3, 4.2.4 Jul 13, 2022
@gu-stav gu-stav marked this pull request as ready for review July 13, 2022 15:45
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

So happy to see moment go :D nice job!

@@ -1008,7 +1008,7 @@ exports[`<CreatePage /> renders and matches the snapshot 1`] = `
id="textarea-1"
name="description"
>
Created 2021–01–30T12:34:56+00:00
Created April 1st, 2020
Copy link
Member

Choose a reason for hiding this comment

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

do we want that change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is ok, for 2 reasons:

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM. Works well 👍

@gu-stav gu-stav merged commit b3d6e8a into master Jul 14, 2022
@gu-stav gu-stav deleted the chore/replace-moment-js branch July 14, 2022 07:54
@gu-stav gu-stav modified the milestones: 4.2.4, 4.3.0 Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: core:admin Source is core/admin package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants