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

Humanize not localised #28331

Open
2 of 3 tasks
lscheibel opened this issue May 3, 2024 · 5 comments
Open
2 of 3 tasks

Humanize not localised #28331

lscheibel opened this issue May 3, 2024 · 5 comments

Comments

@lscheibel
Copy link
Contributor

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

  • I have searched Superset docs and Slack and didn't find a solution to my problem.
  • 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
Copy link
Member

rusackas commented May 3, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

@mistercrunch
Copy link
Member

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.

lscheibel added a commit to lscheibel/superset that referenced this issue May 10, 2024
…28331)

 Fixes: table views, dashboard card and chart editor header
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

No branches or pull requests

4 participants