rusackas commented on PR #39330:
URL: https://github.com/apache/superset/pull/39330#issuecomment-4442264722
Good direction overall — `Intl.DurationFormat` is the right call for native
localization with no bundle cost, and the PR description does a thorough job
justifying the choice.
A few things worth addressing before merge:
**Negative durations are broken (high).** `parse-ms` internally uses
`Math.abs()` so negative inputs silently lose their sign. `Intl.DurationFormat`
requires all fields in the duration object to share the same sign, so you need
to extract the sign before parsing and either apply it to all fields or prepend
`-` to the formatted string. If tests for negative values like `-1s` still
pass, it's worth double-checking they're actually exercising this path.
**Sub-millisecond truncation changes default behavior (medium).** The new
code defaults `formatSubMilliseconds` to `false` and zeroes out ms/µs/ns. This
means `10500 ms` now renders as `10s` instead of `10.5s` — a user-visible
regression for anyone on the existing DURATION format. If this is intentional,
it should be called out explicitly; otherwise `formatSubMilliseconds` should
probably default to `true`.
**Locale normalization is incomplete (easy fix).** `locale?.replace('_',
'-')` only replaces the first underscore. Use `replace(/_/g, '-')` or
`replaceAll('_', '-')` to handle locales like `zh_Hans_CN` correctly.
**Wrong Redux state type in two components (medium).** Both
`TaskList/index.tsx` and `TaskStatusIcon.tsx` use `ExplorePageState` to type
the `useSelector` call for `locale`. This should be `SupersetRootState` (or the
project's root state type) — using a page-specific type in a shared component
hides type errors.
**Formatter instantiation on every call (medium).** `formatDuration` in
`timeUtils.ts` creates a new `Intl.DurationFormat` on every invocation.
Consider memoizing by `(locale, style)` or reusing the registry pattern from
`createDurationFormatter`.
Minor: double slash in the import path `utils//getIntlDurationFormatter` in
the test file.
The `zeroDurationFormatter` fallback, the `Math.round()` ETA fix, and the
`parseMilliseconds` year-support matching `pretty-ms` are all nice touches.
Once the negative-duration handling and the truncation regression are sorted,
this should be in good shape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
