Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

feat: use moment.js for timeAgo #521

Merged
merged 3 commits into from
Nov 20, 2020
Merged

Conversation

akloeckner
Copy link
Contributor

also export the moment class to user space
this will enable the user to do

moment.locale('de');

in their config.js.

also export the moment class to user space
this will enable the user to do
```
moment.locale('de');
```
in their `config.js`.
Copy link
Collaborator

@rchl rchl left a comment

Choose a reason for hiding this comment

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

This adds quite a chunk of data to the bundle (maybe 500kb or so) but I guess it's probably fine.

scripts/globals/utils.js Outdated Show resolved Hide resolved
@akloeckner
Copy link
Contributor Author

I tried to find a way to avoid bundling all the locales. We could only bundle "popular" locales like ru, de, en, pl. Or provide info on how the user can load locales. Both don't seem very appealing to me...

@akloeckner
Copy link
Contributor Author

Actually, timeAgo seems to be used only for the default tooltip in history tiles and the corresponding example. Maybe it's better to not touch that function or remove it altogether...

@akloeckner
Copy link
Contributor Author

So, this is a simple one now... I'll PR i18n separately...

@akloeckner akloeckner mentioned this pull request Nov 12, 2020
@rchl
Copy link
Collaborator

rchl commented Nov 19, 2020

I've briefly looked at using date-fns instead, which is preferred in the community now (even https://momentjs.com/ discourages its use), but date-fns doesn't currently support setting locale globally so it wouldn't be as flexible. We would need to import all languages we want to support up-front (that seems fine) and then all uses of date-fns functions would basically have to be wrapped in a function that uses the chosen locale (which user could define in the config).

I'll wait to hear what you have to say about that before merging this one.

(We would still have to include moment.js for the chart.js library)

@rchl rchl merged commit 5227be4 into resoai:master Nov 20, 2020
@akloeckner
Copy link
Contributor Author

I think, it's good to stick with moment.js until we sort this out once and for all. Since chart.js uses moments we might even end up deciding to stick with it despite its deprecation.

Wrapping in a function wouldn't really stop me from using it, though. Currently timeAgo is the only direct usage of that sort. And it's a wrapper already. (Maybe the functional programming variants would make good wrappers, btw.)

@rchl
Copy link
Collaborator

rchl commented Nov 20, 2020

BTW. If we advertise the use of something like moment.locale('de'), we should rather introduce a config option for it so that we can abstract locale change and potentially replace the underlying implementation in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants