Humanize not localised - Githubissues

lscheibel commented 6 months ago

Bug description

As far as I can tell and from what I'm seeing in the frontend, strings send from the backend to the frontend that have been formatted with humanize are not localised and will always use english. There's a couple of places where these strings are used directly in the frontend but one of them comes from superset/models/helpers.py which is then used in the chart editor in superset-frontend/src/explore/components/ExploreChartHeader/index.jsx.

How to reproduce the bug

Change the superset language to something other than english.
Edit an existing chart.
Take a look at the header and the last-modified label.

Screenshots/recordings

Superset version

master / latest-dev

Python version

3.9

Node version

18 or greater

Browser

Chrome

Additional context

I found flask-humanize which could be a solution. Although a question would be, how much Superset wants to rely on server-side localised strings. Especially considering that moment.js is already bundled which can achieve similar things in the frontend.

For a PR would you rather see something that fixes the localisation problem in the backend or changes that would send the date string to the frontend, where they would be pretty-printed using moment?

Checklist

[X] I have searched Superset docs and Slack and didn't find a solution to my problem.
[X] I have searched the GitHub issue tracker and didn't find a similar bug report.
[ ] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

rusackas commented 6 months ago

Personally, I think doing this in the front end makes more sense, but it's probably debatable! That would make it easier to leverage in the myriad places we'll want to do so going forward.

One thing that's awkward here is the fact that moment.js is no longer maintained. At some point we might want to migrate to date-fns or whatever's cool/stable. To minimize the pain here, it might make sense to centralize the logic here as either a utility function in the codebase utils (get_humanized_time(datetime, {lang}) or perhaps a react component that renders a span with the appropriate string.

As long as we adhere to the DRY principle and reduce/reuse/recycle logic so it's scalable/maintainable, I'm open to whatever works :) I'll just ping @michael-s-molina @dpgaspar @mistercrunch who might have different/additional opinions on the matter.

Thank you for offering to contribute a PR around this!

mistercrunch commented 6 months ago

I think that belongs in the frontend too, ideally backend sends a utc stamp and frontend can show something even relative to now that changes with timers and such. What's the new hot library to replace moment?

hainenber commented 6 months ago

date-fns is both stable and cool for some time. It's lightweight, actively maintained and very much feature-parity with moment.js. Second vote for that sturdy library.

lscheibel commented 6 months ago

Hm, I see moment.js being in maintaince mode as a separate Problem from what I described initially. I definitely see your arguments regarding phasing out moment but as of today, it is pretty deeply rooted within the frontend. Moment is a dependency not just for antd4 but also the echarts- and handlebars-plugins, as well as a few other legacy plugins. Therefore I would stick with moment for now, instead of introducing yet another dependency.

I hope to submit a draft PR this week.