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

chore: tree-shake date-fns package #6602

Closed
wants to merge 1 commit into from

Conversation

JinCoreana
Copy link
Contributor

@JinCoreana JinCoreana commented Feb 22, 2024

Proposed behaviour

Tree-shake dates-fns and reduce bundle size to 252KB

AFTER

Screenshot 2024-02-23 at 09 09 05 Screenshot 2024-02-23 at 09 09 17

Current behaviour

dates-fns includes methods that are not in use to the bundle increasing size to 1.23MB
Open issue: date-fns/date-fns#3004

BEFORE

Screenshot 2024-02-22 at 11 15 16

Screenshot 2024-02-22 at 13 13 10

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

@JinCoreana JinCoreana requested a review from a team as a code owner February 22, 2024 13:48
@JinCoreana JinCoreana self-assigned this Feb 22, 2024
@JinCoreana JinCoreana force-pushed the SBS-84395 branch 3 times, most recently from ce5779f to f3da4ee Compare February 22, 2024 13:59
@JinCoreana JinCoreana marked this pull request as draft February 22, 2024 14:05
Copy link
Contributor

@edleeks87 edleeks87 Feb 22, 2024

Choose a reason for hiding this comment

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

Comment: Carbon is open source, we can't limit these locales here imo, if we did it would need to be a breaking change as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think reverting it would address the linting an playwright failures you're seeing as well

Copy link
Contributor Author

@JinCoreana JinCoreana Feb 22, 2024

Choose a reason for hiding this comment

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

Thank you for the comment :)
Now I will check.

  • solely limiting methods import? ❌- minor improvement
  • downgrading or upgrading date-fns? Downgrade to 2.27.0 ❌- since the primary cause is bundling the entire locales, no improvement / Upgrade ❌ - for the same reason, no improvement but please note: V3 resolved many structural issues and is not compatible withdate-fns-tz yet. open PR: Update date-fns to v3 marnusw/date-fns-tz#265 /
  • lazy-loading date-fns? ➖
  • consumer side webpack configuration? ➖

Copy link
Contributor Author

@JinCoreana JinCoreana Feb 23, 2024

Choose a reason for hiding this comment

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

Hi @edleeks87
I've looked into your comment about the date locales logic. It seems that Carbon only requires en-GB as the default, which is set in the I18nProvider. If users need different date locales, they'll have to install date-fns and pass it as dateFnsLocale of locale prop to the I18nProvider.

Screenshot 2024-02-23 at 03 54 04

I checked if there is any logic for users to simply pass a locale code, and Carbon automatically uses the related date locale which then would involve bundling all date locales, but I couldn't find anything like that. I understand the failing part of the Playwright tests is for testing the dateFnsLocale that consumer passes from their level so I imported the whole date-fns only in test files as it does not seem to increase the bundle size. Could you confirm if I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @JinCoreana thanks for the detailed investigation. We export the locales via Carbon so that projects using it don't also require having date-fns installed. The easiest win here might be to move this out of carbon, add date-fns as a peer dependency and then require consuming projects to import and use the locales they want. We would need to do it as a breaking change so my preference is to leave this file for now and discuss it with the Carbon team as there may well be other changes we'd like to make and it would be better for all that to be bundled together as one new major version imo

Copy link
Contributor Author

@JinCoreana JinCoreana Feb 23, 2024

Choose a reason for hiding this comment

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

I see. Thank you, Ed! Would it be good that we open a new issue for your discussion then?
I will be off next week but I handed over this PR to @AlexKryzh, I will still ask him to explore other approaches like lazy loading or consumer side adjustments to see if we can make improvements in the meantime even with the entire locales in the bundle. We will keep you posted on this. :)

Copy link
Contributor

@AlexKryzh AlexKryzh Feb 26, 2024

Choose a reason for hiding this comment

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

Thanks @edleeks87 , this was a Miro's team spike ticket of 2-3 days to investigate the best solution.
Can you confirm that Carbon Team prefer to do a refactor? Refactor about don't include all locations and permit to consumers include required locations.
Just for extra info: our next step was to investigate to lazy load this locations.

Copy link
Contributor

Choose a reason for hiding this comment

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

@edleeks87 will discuss this changes with the Carbon Team, probably it will be part of breaking change Carbon update.

@JinCoreana JinCoreana force-pushed the SBS-84395 branch 2 times, most recently from 73486f3 to 8db1290 Compare February 23, 2024 08:08
hi,
sl,
lv,
} from "date-fns/locale";
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 have added this index only for playwright wondering date.pw test is flaky because of latency caused by importing the entire locale. So I've configured it to import only the necessary testing locales.

@JinCoreana JinCoreana removed their assignment Feb 23, 2024
Copy link
Contributor

@edleeks87 edleeks87 left a comment

Choose a reason for hiding this comment

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

If we're no longer going to re-export the date-fns locales we should add it as a peerDependency so consumers have it installed in their own node_modules. I think the commit message should look something like this as well:

fix: ensure that tree-shaking works with date-fns package 

Updates the import paths for `Date` utils to allow tree-shaking to work and a reduction in bundle size.
    
BREAKING CHANGE: Carbon no longer re-exports the `date-fns` locales, consuming projects will need to install the package and use them directly from there.

closes: SBS-84395

Copy link
Contributor

Choose a reason for hiding this comment

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

comment: I'm happy with this change. We'll need to do it as a breaking change though as anyone who's imported from this file will obviously need to update and install date-fns

@nineteen88
Copy link
Contributor

nineteen88 commented Feb 27, 2024

I am content with the changes proposed. I will approve it once we've addressed Ed's comments as I think it's important.

@nineteen88 nineteen88 self-requested a review February 27, 2024 14:26
@JinCoreana
Copy link
Contributor Author

JinCoreana commented Mar 1, 2024

Hello, I am back and caught up with @AlexKryzh
According to his conversation with @edleeks87, this PR is closed as Carbon team will take it over and address this breaking changes. Thank you!
cc: @nineteen88

@JinCoreana JinCoreana closed this Mar 1, 2024
@nineteen88
Copy link
Contributor

@JinCoreana This has now been merged in the following PR: #6713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants