[Fiche navire] Rechercher les JPEs par numéro de marée

[louptheron]

Cette PR introduit l'organisation par features à titre d'exemple et de discussion :

La partie encadrée correspond au composant VesselLogbook, importé par VesselSidebar:

Linked issues

• Resolve Rechercher les JPEs par numéro de marée #2112

---

• Tests E2E (Cypress)

[ivangabriele]

Nommage

• ALL_UNDERSCORED_CAPS pour les constantes qui sont dans le scope global (bon sur cette PR mais précisé pour Env)
  • https://www.syncfusion.com/blogs/post/10-javascript-naming-conventions-every-developer-should-know.aspx
  • https://github.com/airbnb/javascript#naming--uppercase (leurs règles sont précises à ce sujet)
  • Les enums échappent évidemment à cette règle puisqu'ils sont déjà conventionnés par TS en PascalCase
• On a un mélange de dossiers camelCase / snake_case. Ca serait pas mal de suivre la camelCase partout, ce qui est une convention "empirique" en JS. Du coup je propose useCases plutôt que use_cases ici. Pour les tests e2e, à voir, mais le snake_case ne me dérange pas dans les tests e2e car ils sont à part.

Types

• Je propose qu'on namespace ce qui est dérivé des entités. Ces namespaces devraient normalement du coup correspondre aux dossiers que l'on trouve dans backend/.../domain/entities. Ce qui veut dire que LogBookMessage devient LogBook.LogBookMessage sur cette PR par exemple. On le fait déjà un peu sur Fish même si ce n'est pas le cas sur cette PR.
• Si l'on s'accorde sur cette proposition, ça ouvre peut-être la question d'avoir soit un sous-dossier types pour séparer le domaine des types "fonctionnels", soit d'avoir un fichier type à part lorsqu'il s'agit d'un namespace (ex : LogBook.types.ts). Petite préférence personnelle pour la seconde solution.
• Je profite de ce sujet pour proposer un pattern de nommage correspondant à un CRUD typique de l'API (en mode DTO) :

export namespace Foo {
  // Ce qu'on reçoit via un GET
  export interface Bar {
    childIds: number[]
    children: Other.Child[]
    id: number
    name: string
    parent: Other.Parent
    parentId: number
  }

  // ---------------------------------------------------------------------------
  // Types

  // Ce qu'on envoie via un PUT
  export type BarData = Omit<Bar, 'children' | 'parent'>
  // Ce qu'on envoie via un POST
  export type NewBarData = Omit<BarData, 'id'>
}

Redux

• J'ai discuté avec Loup, puis Claire de ça : autant regrouper les dispatchers et les reducers via une même constante myActions qu'on exporte en une seule ajoute de l'indexation inutile et déclarifie la différence entre les deux, autant pour les reducers seuls, j'aurais tendance à toujours préférer n'avoir qu'une une seule constante export const fooActions = fooSlice.actions pour clarifier la compréhension du code. Sachant qu'on utilise souvent les actions de plusieurs slices. Il y a aussi beaucoup d'actions, du coup le namespacing facilite la différenciation sans compliquer le naming, par ex : sideWindowActions.showModal() VS showSideWindowModal(). En bonus, ça réduit le nombre des imports.

Test unitaires

• J'aurais tendance à préférer avoir une fichier par fonction testée (plutôt que le utils.test.ts sur cette PR par exemple).

[louptheron]

Globalement d'accord avec toi @ivangabriele.

Quelques retours :

• J'ai modifié en useCases
• Ok pour le namespace, mais il faut s'accorder sur ce qui est le "core business", car dans domain on avait justement parfois des mixs. Perso je dirais qu'on namespace ce qui provient de l'API, wdyt ?
  • Je n'arrive pas à refacto avec mon IDE en namespace (ça touche beaucoup de fichiers) donc pour l'instant j'ai juste migré les types à namespacer dans logbook.types.ts.
• Ok pour les types du CRUD
• ça me va également de regrouper les actions, c'est testé sur cette PR et c'est plus lisible je trouve.
• Pour les fichiers pour chaque fonction de utils, ça me fait faire 10 fichiers avec à chaque fois 1 test, j'ai du mal à voir le bénéfice dans ce cas précis (je pense que si on a un découpage des tests, on doit aussi avoir un découpage des utils en différents fichiers dans un dossier utils).

[ivangabriele]

@louptheron

• Ça fait sens en effet pour les utils. Entendu pour garder tout dans un utils.test.ts. On ne teste de toute manière que les utils qui incluent des logiques abstraites et/ou complexes.
• D'un point de vue Frontend en effet on n'est pas sensé se préoccuper de la sauce interne du Backend. Je suis d'accord pour plutôt définir nos namespaces par rapport à l'API. Et c'est plus en phase avec le "typage CRUD".
• Juste du chipotage mais totalement optionnel si ça n'a pas de sens pour l'un de nous : je nommerais Logbook.types.ts en PascalCase pour refléter la casse du namespace.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
83 Code Smells

No Coverage information
0.0% Duplication