superset icon indicating copy to clipboard operation
superset copied to clipboard

Humanize not localised

Open lscheibel opened this issue 9 months ago • 5 comments

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

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

Screenshots/recordings

image image

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.

lscheibel avatar May 03 '24 08:05 lscheibel

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!

rusackas avatar May 03 '24 16:05 rusackas

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?

mistercrunch avatar May 04 '24 00:05 mistercrunch

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.

hainenber avatar May 05 '24 15:05 hainenber

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.

lscheibel avatar May 07 '24 07:05 lscheibel

It'd be good to dig deeper, but while moment is used in many places in the codebase (looks like about 30 files), I don't think we use much of the api surface there. I also remember fighting with it around the fact that it was bloating our bundles quite a bit. The fact it isn't maintained is always a risk too. Given all this it seems we should replace as opposed to putting more work on top of it.

mistercrunch avatar May 07 '24 22:05 mistercrunch