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

Bug: Błędy typów w kodzie pomimo Typescripta #1438

Open
MusicFreak456 opened this issue Feb 4, 2023 · 3 comments · May be fixed by #1759
Open

Bug: Błędy typów w kodzie pomimo Typescripta #1438

MusicFreak456 opened this issue Feb 4, 2023 · 3 comments · May be fixed by #1759
Assignees

Comments

@MusicFreak456
Copy link
Contributor

Pracując przy komponentach Vue zauważyłem, że jest możliwość popełnienia w nich błędu typów pomimo użycia Typescripta.
Np. w widgecie z powiadomieniami, na ten moment zlokalizowanym tutaj: link. Dało się (a nawet obecnie tak właśnie jest w kodzie produkcyjnym) utworzyć metodę deleteOne(i: number): Promise<void>, a następnie wywołać ją z argumentem typu string: deleteOne(elem.id), gdzie elem jest instancją klasy Notification, a id jej polem typu string.

@Pawe16 Pawe16 self-assigned this Oct 23, 2024
@Pawe16
Copy link
Collaborator

Pawe16 commented Oct 30, 2024

Problem wynika z użycia funkcji infer bilbioteki zod przy deklaracji typu Notification.

// Defines a notification scheme to validate and parse Notifications from JSON.
const notificationScheme = z
  .object({
    id: z.string(),
    description: z.string(),
    issued_on: z.string(),
    target: z.string(),
  })
  .transform((parsedObject) => {
    return {
      id: parsedObject.id,
      description: parsedObject.description,
      issuedOn: parsedObject.issued_on,
      target: parsedObject.target,
    };
  });
type Notification = z.infer<typeof notificationScheme>;

Typ wydedukowany w ten sposób nie zapewnia sprawdzania typów dla swoich pól - z tego powodu można przekazac argument typu string (w kodzie jest to elem.id) do funkcji przyjmujacej argumenty typu number. Jest to jedyne miejsce w kodzie calego projektu, gdy wykorzystywana jest funkcja infer. Po zamianie definicji typu na nastepująca:

type Notification = {
  id: string;
  description: string;
  issuedOn: string;
  target: string;
};

sprawdzanie typów działa ponownie dobrze, próba wywołania deleteOne(elem.id) nie powiedzie sie - zostaniemy poinformowanie o błedzie Argument of type 'string' is not assignable to parameter of type 'number'

@lgpawel
Copy link
Contributor

lgpawel commented Nov 5, 2024

(Śmieszna sprawa – kiedy błąd był zgłaszany, nie używaliśmy jeszcze zod, tylko spicery, czyżby błąd przetrwał przenosiny z jednej biblioteki na drugą?)

Typ wydedukowany w ten sposób nie zapewnia sprawdzania typów dla swoich pól - z tego powodu można przekazac argument typu string

Jak to się ma do faktu, że notificationScheme (też zresztą deklarowane przy użyciu funkcji bibliotecznych zod) wygląda na ręcznie zadeklarowane jako składające się z czterech stringów? Być może te dwie rzeczy są od siebie niezależne, ale jednak trochę wygląda na to, że jedna może wpływać na drugą.

Zaproponowana zmiana w kodzie zawiera zduplikowaną logikę i to oczywiście nie jest idealne. Czy pozostawienie infer (być może z jakąś mniej rozbudowaną modyfikacją) razem z wprowadzeniem wywołania parseInt nie będzie OK, skoro tak czy siak elem.id jest napisem? A jeśli nie, to czy możemy to jakoś inaczej zdeduplikować z powrotem, np. najpierw definiując Notification, a dopiero potem notificationScheme, ale bez czterokrotnego odwołania do z.string(), tylko do Notification właśnie?

@Pawe16
Copy link
Collaborator

Pawe16 commented Nov 12, 2024

Z tego co się zorientowałem spicery i zod są dość podobnymi bibliotekami, dlatego wyjade mi się prawdopodobne, że błąd występuje w obu przypadkach. Natomiast jeśli chodzi o ręczną definicję typów w tym fragmencie kodu

.object({
    id: z.string(),
    description: z.string(),
    issued_on: z.string(),
    target: z.string(),
  })

, to jeśli dobrze rozumiem sposób w jaki działa zod, jest to tylko opis jakiego formatu danych oczekujemy, tzn np przy wywołaniu .then((r) => notificationSchemeArray.parse(r.data)) zostanie przeprowadzona walidacja czy r jest tablicą obiektów składających się z pól id, description, issued_on i target, wszystkich typu string. Zatem zdefiniowany ręcznie typ z.string() ma znaczenie, ale walidacja odbywa się dynamicznie, w trakcie trwania programu. Poza tym na już zwalidowanym obiekcie jest wykonywana transformacja

.transform((parsedObject) => {
    return {
      id: parsedObject.id,
      description: parsedObject.description,
      issuedOn: parsedObject.issued_on,
      target: parsedObject.target,
    };

, i tu typy już nie są zadeklarowane. Wydawałoby się naturalne, gdyby pola obiektu po transformacji również były typu string, ale wygląda na to że są typu any i podjęte przeze mnie próby jawnej definicji typów np:

    return {
      id: parsedObject.id as string,
      description: parsedObject.description as string,
      issuedOn: parsedObject.issued_on as string,
      target: parsedObject.target as string,
    };

nie rozwiązały problemu. Wydaje mi się, że typy są więc sprawdzane tylko dynamicznie. Jeśli chodzi o uniknięcie duplikacji kodu, to moim zdaniem zostawienie funkcji infer i wywołanie funkcji deleteOne z parametrem parseInt(elem.id) będzie dobrym rozwiązaniem. Nie rozwiązuje to stricte problemu tego issue, bo Typescript wciąż nie będzie informował nas o przypadkach, gdy takiego parseInta ktoś zapomniałby napisać, ale w tym przypadku (a jest to jedyny taki przypadek w kodzie) nie wydaje się to niebezpieczne.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants