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

FE-6858 - update date-fns from ^2.30.0 to ^4.1.0 #7156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

quinnturner
Copy link

@quinnturner quinnturner commented Jan 15, 2025

Closes: #7004

Proposed behaviour

Updates date-fns

Current behaviour

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

The main concern with my current implementation is that <DayPicker has a property locale that accepts a date-fns localize type.

weekdaysLong and weekdaysShort are now not accepted properties.

I did not know what to do in this scenario, so I removed the code entirely. However, I suspect that's not the correct behaviour.

I would appreciate guidance on how to proceed.

Testing instructions

@quinnturner quinnturner requested review from a team as code owners January 15, 2025 19:32
@quinnturner quinnturner mentioned this pull request Jan 15, 2025
1 task
@quinnturner quinnturner changed the title chore: fe-6858 - update date-fns from ^2.30.0 to ^4.10.0 FE-6858 - update date-fns from ^2.30.0 to ^4.10.0 Jan 15, 2025
@Parsium Parsium self-requested a review January 20, 2025 15:47
@@ -245,9 +238,6 @@ export const DatePicker = ({
locale={{
localize: {
...defaultLocale.localize,
months: monthsLong,
Copy link
Author

Choose a reason for hiding this comment

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

I really am not sure what to do here. There's a good chance this isn't the best behaviour. Looking for advice/opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

The visible labels and ARIA labels look consistent with production, so this should be fine. We have passing tests that check the weekday labels are two characters long in German and Polish, so this edge case is covered :)

Copy link
Contributor

@Parsium Parsium left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @quinnturner! This should eliminate another piece of tech debt for us. I'll mark this as ready for QA.

Before we release, since this involves a major version upgrade of a dependency, we should mark it as a breaking change. Could you update the commit message to reflect this please? The message will be used in our release notes.

Here's a suggestion:

chore: upgrade `date-fns` to version ^4.1.0


BREAKING CHANGE: `date-fns` is now ESM-first. While CommonJS
remains supported, this change may cause issues in some environments.
If you encounter issues, you may need to update your bundler
configuration to support ESM code.

@@ -245,9 +238,6 @@ export const DatePicker = ({
locale={{
localize: {
...defaultLocale.localize,
months: monthsLong,
Copy link
Contributor

Choose a reason for hiding this comment

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

The visible labels and ARIA labels look consistent with production, so this should be fine. We have passing tests that check the weekday labels are two characters long in German and Polish, so this edge case is covered :)

@Parsium Parsium changed the title FE-6858 - update date-fns from ^2.30.0 to ^4.10.0 FE-6858 - update date-fns from ^2.30.0 to ^4.1.0 Jan 22, 2025
BREAKING CHANGE: `date-fns` is now ESM-first. While CommonJS
remains supported, this change may cause issues in some environments.
If you encounter issues, you may need to update your bundler
configuration to support ESM code.
@quinnturner
Copy link
Author

Thanks, @Parsium; I've amended the commit message to match yours verbatim.

@edleeks87
Copy link
Contributor

Do not merge until #7148 has been released

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.

Update date-fns to v4
6 participants