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

🐞 - Providing TUI_NUMBER_FORMAT with useClass causes incorrect parsing of input #2578

Closed
3 of 9 tasks
pwespi opened this issue Sep 2, 2022 · 6 comments
Closed
3 of 9 tasks
Assignees
Labels
P3 This issue has low priority S2 This issue has medium severity v4 4.0 candidate

Comments

@pwespi
Copy link
Contributor

pwespi commented Sep 2, 2022

Which @taiga-ui/* package(s) are the source of the bug?

core

Please provide a link to a minimal reproduction of the bug

https://github.com/pwespi/tui-input-number

Is this issue blocking you?

Blocking

Description

We are providing TUI_NUMBER_FORMAT with useClass (to be able to change the format dynamically). This causes incorrect parsing of the input when the input loses focus.

Screen Recording 2022-09-02 at 16 46 04(1)

This used to work in Taiga UI 2.x.

Angular version

^14.1.0

Taiga UI version

^3.0.1

Which browsers have you used?

  • Chrome
  • Firefox
  • Safari
  • Edge

Which operating systems have you used?

  • macOS
  • Windows
  • Linux
  • iOS
  • Android
@pwespi pwespi added bug labels Sep 2, 2022
@splincode splincode added P3 This issue has low priority contributions welcome Architecture is clear and community can help community contribution This issue was closed by a PR from the community and removed state: need triage community contribution This issue was closed by a PR from the community labels Sep 5, 2022
@ddubrava
Copy link
Contributor

ddubrava commented Sep 17, 2022

Hey, guys! @splincode  @nsbarsukov

The main reason of this problem is that we use spread operator to pass settings. Since spread operator doesn't include accessors (see microsoft/TypeScript#26547), we lost all the settings.

image

The workaround is to copy everything (including accessors) from the settings to an object but I'm not sure it's a great idea to hardcode this.

@waterplea
Copy link
Collaborator

Well, this kind of spreading is used everywhere, we cannot take such cases into account in all situations. Can you show the code of your class? Let's try to think of the best workaround together.

@ddubrava
Copy link
Contributor

@waterplea
Copy link
Collaborator

@ddubrava
Copy link
Contributor

@waterplea https://github.com/pwespi/tui-input-number/blob/main/src/app/app.component.ts
Screenshot 2022-09-27 at 16 18 39

What is the point of it being a getter, then?

It's not my code, I don't know :d @pwespi
I guess it's just the minimum reproduction, in real code there might be more complicated logic.

@pwespi
Copy link
Contributor Author

pwespi commented Oct 3, 2022

Thank you for looking into this and sorry for the late response.

To give some context, our goal is to allow the user to change the locale of the app at runtime, and as such change the formatting of numbers dynamically without reloading the app.

In Taiga UI 2 we solved this by providing TUI_NUMBER_FORMAT in the app module using useClass and something like

{
      provide: TUI_NUMBER_FORMAT,
      useClass: TuiNumberFormat,
      deps: [LocaleService],
}
export class TuiNumberFormat implements TuiNumberFormatSettings {
  constructor(private localeService: LocaleService) {}

  get decimalSeparator(): TuiDecimalSymbol {
    return this.localeService.tuiNumberFormat.decimalSeparator;
  }

  get thousandSeparator(): string {
    return this.localeService.tuiNumberFormat.thousandSeparator;
  }

  get zeroPadding(): boolean {
    return this.localeService.tuiNumberFormat.zeroPadding;
  }
}
export const TUI_NUMBER_FORMAT_MAP: {
  [key in Locale]: TuiNumberFormatSettings;
} = {
  'en-US': {
    decimalSeparator: '.',
    thousandSeparator: ',',
    zeroPadding: false,
  },
  'en-GB': {
    decimalSeparator: '.',
    thousandSeparator: ',',
    zeroPadding: false,
  },
  'de-DE': {
    decimalSeparator: ',',
    thousandSeparator: '.',
    zeroPadding: false,
  },
  'de-CH': {
    decimalSeparator: '.',
    thousandSeparator: "'",
    zeroPadding: false,
  },
  // ...
};

@Injectable({
  providedIn: 'root',
})
export class LocaleService {
  locale: BehaviorSubject<Locale> = new BehaviorSubject<Locale>('en-US');

  constructor(
    private settingsRepository: SettingsRepository,
    private errorService: ErrorService
  ) {}

  get tuiNumberFormat(): TuiNumberFormatSettings {
    return TUI_NUMBER_FORMAT_MAP[this.locale.value];
  }

  // ...
}

This stopped working when we upgraded to Taiga UI 3 in an unexpected way (see gif in issue description). That's why I created this issue with a minimal reproduction.

We since refactored our code to provide TUI_NUMBER_FORMAT in components that need it using:

{
      provide: TUI_NUMBER_FORMAT,
      useFactory: tuiNumberFormatFactory,
      deps: [LocaleService],
}
export const tuiNumberFormatFactory = (
  localeService: LocaleService
): TuiNumberFormatSettings => {
  return {
    decimalLimit: localeService.tuiNumberFormat.decimalLimit,
    decimalSeparator: localeService.tuiNumberFormat.decimalSeparator,
    thousandSeparator: localeService.tuiNumberFormat.thousandSeparator,
    zeroPadding: localeService.tuiNumberFormat.zeroPadding,
  };
};

This works in Taiga UI 3, so this issue is no longer blocking for us, but we need to provide the options in many places.

One alternative solution I can think of is for tokens like TUI_NUMBER_FORMAT to allow the type InjectionToken<Observable<TuiNumberFormatSettings>> in addition to InjectionToken<TuiNumberFormatSettings> (similar to TUI_LANGUAGE), but I have no idea how much work that would be.

@waterplea waterplea added the S2 This issue has medium severity label Mar 31, 2023
@github-project-automation github-project-automation bot moved this to 💡 Backlog in Taiga-family Sep 25, 2023
@vladimirpotekhin vladimirpotekhin added the v4 4.0 candidate label Feb 29, 2024
@github-project-automation github-project-automation bot moved this from 💡 Backlog to ✅ Done in Taiga-family Feb 29, 2024
@waterplea waterplea removed the contributions welcome Architecture is clear and community can help label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 This issue has low priority S2 This issue has medium severity v4 4.0 candidate
Development

No branches or pull requests

5 participants