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

Enable moment-timezone functionality #216

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

Conversation

MeeDNite
Copy link

No description provided.

Copy link
Member

@alitaheri alitaheri left a comment

Choose a reason for hiding this comment

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

I'm sorry, but don't agree with this change at all. moment-timezone is a huge library and it will impact many applications using moment-jalaali. please either fork this project for your use case, or add a functionality that would replace the installed moment with the moment of your choice, in your case moment-timezone.

@MeeDNite
Copy link
Author

MeeDNite commented Mar 27, 2021

I'm sorry, but don't agree with this change at all. moment-timezone is a huge library and it will impact many applications using moment-jalaali. please either fork this project for your use case, or add a functionality that would replace the installed moment with the moment of your choice, in your case moment-timezone.

Hi,

moment itself doesn't allow us to set default timezone, thus when using moment-jalaali, timezone always depends on client computer's timezone.

As shown here, we can set default timezone only using moment-timezone.

Furthermore moment-timezone is already a dependency here

@behrang
Copy link
Member

behrang commented Sep 19, 2022

Hey @alitaheri,

I think we can accept this change, since this should not change the dependency structure and we can also close #242 too. What's your opinion?

Also, tree shaking tools can remove unused code in production deployments. So client-side developers can rely on such tools to minimize bundle sizes, can't they?

@behrang
Copy link
Member

behrang commented Sep 24, 2022

Ok, I read a little about moment-timezone. As I understand it, it is a library that helps mainly with parsing and formatting of time zones. The timezone data is separate and must be loaded before use. In node environment it is already loaded, but on client, it must be downloaded to be accessible to the library.

Now, I quickly changed the require line as was suggested by @MeeDNite, and all tests passed successfully. However, I don't know the result of this change in client side.

@MeeDNite, @malikarami Can you check the side-effects on the client side, so we can come to a conclusion?

The target is that current users of this library should not notice a huge change in file size, and the change must not break any existing code.

The final result is that tz function will be available on the server and client and can be used to change timezone of formatted moments.

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.

3 participants