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

Explicitly provide resolved locale for all functions which need it #2523

Closed
wants to merge 1 commit into from

Conversation

daveallie
Copy link
Contributor

What's Changed

locale is optionally user defined as a prop to DayPicker and should fallback to en-US (the default for DayPicker) when none is supplied by the user. There were several places where this default was not respected, and instead props.locale was used (which can be undefined).

This PR addresses the above problem by explicitly requiring locale as a non-optional argument everywhere it's used, similar to how dateLib is passed around.

This resolves #2509 (related discussion #2511) in where if the client application used date-fns and had a locale set through dateFns.setDefaultOptions, then the internal calls to date-fns within DayPicker would use this client defined default (instead of the en-US default) in the cases where props.locale was being used (namely useCalendar).

Once we cross the threshold into DayPicker, we should always be using a resolved locale object to ensure consistency of rendered dates across components.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Additional Notes

Fixes #2509
Related #2511

@gpbl
Copy link
Owner

gpbl commented Oct 9, 2024

@daveallie, thanks for your PR! I see your point.

I need to investigate this issue further. I assumed date-fns' setDefaultOptions worked per package and it surprised me it "leaked" into DayPicker.

I planned to use setDefaultOptions in DayPicker too, but if it changes the behavior of any date-fns used in the application, it might not be the right solution. We may need to add locale as you did here.

@daveallie
Copy link
Contributor Author

yep, unfortunately it's not like Axios where you can create an instance and then configure it with all your relevant defaults, setDefaultOptions applies to the whole library (although I suspect if you have multiple versions of date-fns available to your application, you'd get some interesting behaviour depending on where you called it from).

the options as I see it are:

  1. Be diligent with ensuring locale is defined everywhere you call a date-fns function
  2. Re-export each date-fns method from your own module with a required locale prop to ensure usage
  3. Build a module / class which takes locale at initialisation and sets up functions using date-fns' FP currying (or just have that module/class internally call the date-fns methods with the locale)

imo 3 makes the most sense as dateLib is already being passed around. I would consider defining a specific class to wrap all date-fns calls along with the locale and replace all the dateLib passing with an instance of this new class.

e.g.

class DateUtils {
  constructor(readonly locale: Locale) {}

  format(date: Date, formatStr: string, options: FormatOptions = {}) {
    return format(date, formatStr, { ...options, locale: this.locale });
  }

  // ...
}

const dateLib = new DateUtils(locale);
dateLib.format(new Date(), "PPPP");

@gpbl
Copy link
Owner

gpbl commented Oct 10, 2024

I really like the idea of a DateLib class initialized with options, rather than constantly passing locale. However, I'm unsure if we should merge this or undo all your work, @daveallie.

First, I'd suggest checking whether using a DateLib class introduces any breaking changes (it shouldn't), and if not, go ahead and implement the class right away.

@daveallie
Copy link
Contributor Author

I don't mind at all if the work so far gets undone, the fix isn't urgent as the issue requires a few different stars to align (apparently just being Australian is enough 😅). I should have some time this weekend to implement it if you'd like to leave it in my hands, otherwise if you'd like to take this over, then you're more than welcome to

@daveallie
Copy link
Contributor Author

daveallie commented Oct 12, 2024

Diving into this, it's a fair bit more involved than I initially suspected, opening a new PR in case we want to merge this instead. I'll fill out the PR body details a little later: #2538

@gpbl
Copy link
Owner

gpbl commented Oct 23, 2024

Superseded by #2550

@gpbl gpbl closed this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent weekday headers and grid dates when using date-fns with default locale
2 participants