jquense / react-big-calendar

gcal/outlook like calendar component
http://jquense.github.io/react-big-calendar/examples/index.html
MIT License
7.85k stars 2.23k forks source link

custom timezone (moment) #2400

Open ariel22411 opened 1 year ago

ariel22411 commented 1 year ago

Clear and concise description of the problem

I don't want to change all project time zone by moment.tz.setDefault().

Suggested solution

pass timezone when creating momenLocalizer const localizer = momentLocalizer(moment, timezone);

Alternative

No response

Additional context

No response

Validations

Would you like to open a PR for this feature?

cutterbl commented 1 year ago

@ariel22411 So you're saying that your calendar will use one timezone, but you would have other date elements, on your active screen, using a different timezone? Theoretically?

ariel22411 commented 1 year ago

@cutterbl Let's say there is a customer who lives in Los Angeles, and he is looking at his calendar and sees an event between 8 AM and 9 AM. He reports to you that there is some issue. You connect to his user account, but you are located in France, and the times you see on the calendar are different because your time zone is France and his is Los Angeles. So, once we adjust the time zone that you want to display on the calendar, the issue will be resolved.

cutterbl commented 1 year ago

@ariel22411 I follow so far, I just don't understand your issue. So, you connect to this user's account. Your localizer setup (typically in a useMemo of some kind) would include moment.tz.setDefault(user.timezone), setting all dates in the calendar to use the user's timezone. Somewhere you would also have

// reset default timezone when interface is unmounted
useEffect(() => {
  return () => {
    moment.tz.setDefault()
  }
}, [user.timezone])

Am I missing something? I think our timezone example code even included similar. (Not perfect, but should've been good gist of how to handle...)

ariel22411 commented 1 year ago

The moment.tz.setTimezone function is not recommended for several reasons:

1) Global impact: Calling moment.tz.setTimezone affects the timezone globally for your entire application. This can lead to unexpected behavior and make it difficult to manage and debug issues related to timezones.

2) Concurrency issues: If you have multiple components or modules that rely on different timezones, setting the timezone globally can create conflicts and inconsistencies. It becomes challenging to maintain accurate and reliable time calculations.

3) Unpredictable side effects: Changing the timezone globally can have unintended consequences, especially if you have dependencies or libraries that rely on the default timezone. It can affect other parts of your codebase, causing confusion and introducing bugs that are hard to trace.

Using the moment.tz.setTimezone function is very dangerous, even though I initialize it when I unmount the component. I have a dropdown with a list of teams, and each team can be from a different country. I prefer to pass a timezone prop instead of changing the timezone for the entire application.

cutterbl commented 1 year ago

@ariel22411 OK, I can see some valid concerns. Current pattern was adopted because moment uses an entirely different method for handling dates cast to a timezone (moment.tz(date, timezone), as opposed to just moment(date), which would just use the default). Since not all users of moment are using moment-timezone, this seemed the easiest way to handle this, as opposed to verifying moment.tz availability in every call (and, considering Big Calendars typical usage case). But, I can see where your concerns are.

As a viable alternative, I could see creating a separate momentTimezoneLocalizer, specifically for this use case. This would ensure backwards compatibility, for those with no issue in the current context, or using moment without moment-timezone, while covering your specific end case scenario. The localizer constructor could take an optional timezone prop, defaulted to browser local using moment.tz.guess(). I would think a makeMoment(date) method might simplify all of the other calls throughout the localizer.

We can consider this for our 'next' project (next major release). This is still in it's infancy stages, so it may be a while before it's available. However, if you need it sooner you could submit a PR, creating a new momentTimezoneLocalizer from a copy of the momentLocalizer. I don't personally have time to work on this right now, but would be happy to review a PR. If you do decide to go this route, please review our Contribution Guidelines

canopus1io commented 1 year ago

@ariel22411 @cutterbl We have been using this proposed localizer(PR #2406) for months now, and its been just fine up until recently we realized there is some issues with Daylight Savings Time, I commented with details, examples, and a potential fix on this PR: #2406