krishn1301 opened a new issue, #43430:
URL: https://github.com/apache/superset/issues/43430
### Bug description
`createSmartNumberFormatter()` discards a caller-supplied `id` and returns a
formatter whose `id` is `SMART_NUMBER_SIGNED`, even when `signed` was never
set. The resulting object is internally inconsistent: it identifies as the
*signed* formatter but does not sign anything.
The `id` and `label` options are part of the exported public API of
`@superset-ui/core`, so any plugin registering its own adaptive formatter hits
this.
### How to reproduce
```ts
import { createSmartNumberFormatter } from '@superset-ui/core';
const f = createSmartNumberFormatter({ id: 'my_custom_format', label: 'My
format' });
f.id // 'SMART_NUMBER_SIGNED' <- expected 'my_custom_format'
f(5) // '5' <- no '+', so it is not actually a
signed formatter
f.label // 'My format' <- label is handled correctly
```
Run against `9f505eb0`:
```
passed id 'my_custom_format', signed omitted -> formatter.id =
'SMART_NUMBER_SIGNED'
formats 5 as '5' (no '+', so it is NOT a signed formatter)
```
### Expected results
`f.id === 'my_custom_format'`, and the `signed` flag alone decides between
`SMART_NUMBER` and `SMART_NUMBER_SIGNED` when no `id` is supplied.
### Actual results
The custom `id` is dropped and replaced with `SMART_NUMBER_SIGNED`.
### Root cause
`superset-frontend/packages/superset-ui-core/src/number-format/factories/createSmartNumberFormatter.ts`:
```ts
id:
id || signed
? NumberFormats.SMART_NUMBER_SIGNED
: NumberFormats.SMART_NUMBER,
label: label ?? 'Adaptive formatter',
```
`||` binds tighter than `?:`, so this parses as:
```ts
id: (id || signed) ? SMART_NUMBER_SIGNED : SMART_NUMBER
```
Any truthy `id` therefore selects the *signed* branch instead of being used
as the id. The intended grouping is almost certainly:
```ts
id: id ?? (signed ? NumberFormats.SMART_NUMBER_SIGNED :
NumberFormats.SMART_NUMBER),
```
The `label` line immediately below already uses `??` for exactly this
"caller value wins, otherwise fall back" shape, which suggests the `id` line is
a precedence slip rather than intent.
Truth table for the current expression:
| `id` | `signed` | current `.id` | expected |
| --- | --- | --- | --- |
| omitted | `false` | `SMART_NUMBER` | `SMART_NUMBER` |
| omitted | `true` | `SMART_NUMBER_SIGNED` | `SMART_NUMBER_SIGNED` |
| `'my_fmt'` | `false` | `SMART_NUMBER_SIGNED` | `'my_fmt'` |
| `'my_fmt'` | `true` | `SMART_NUMBER_SIGNED` | `'my_fmt'` |
Only the two default rows are correct, and those are the only two the
current tests and internal callers exercise.
### Scope
Formatting output is **not** affected — `formatFunc` reads `signed`
directly, so numbers render correctly in every case. The damage is confined to
the formatter's identity:
- A plugin registering the formatter under its own id gets an object whose
`.id` disagrees with its registry key.
- Anything keying off `.id` to decide whether a formatter is signed will be
wrong.
Both in-tree call sites (`NumberFormatterRegistry`) pass no `id`, so
**nothing in Superset itself misbehaves today**. This is a latent defect in
exported API surface.
The existing suite
(`test/number-format/factories/createSmartNumberFormatter.test.ts`) never
passes `id` or `label`, which is why it stays green.
### Environment
- master at `9f505eb0`
- Reproduced with the repo's own Jest setup, no patches
### Checklist
- [x] I have searched Superset's GitHub issues and pull requests and found
no report of this
- [x] I have reproduced the issue on current `master`
--
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]
