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

Refactoring de live en workingVersion #458

Merged
merged 50 commits into from
Dec 1, 2021
Merged

Refactoring de live en workingVersion #458

merged 50 commits into from
Dec 1, 2021

Conversation

thom4parisot
Copy link
Member

@thom4parisot thom4parisot commented Nov 18, 2021

Annule et remplace #454

Ça me branche bien de profiter de l'hiver pour tailler les branches, et partir dans un travail de refactoring qui allège l'application, de la technique aux concepts. L'écran d'écriture est un endroit où on constate des nœuds de performance, où tout est intriqué et où ça demande beaucoup de souplesse pour développer sans alourdir à peu près tout.

Il y 2 collections articles et versions, et cette dernière contient des versions dites autosave (la copie de travail toujours affichée à l'écran) et les versions mineurs/majeures (des archives en lecture seule).

Plutôt que de distinguer les natures de versions, on bouge vers un concept où :

  • l'article contient toutes les données courantes, modifiables
  • les versions sont purement en lecture seule

Ça nous sera utile pour le collaboratif temps-réel, et pour tout le reste.

  • ajout de workingVersion au modèle
  • remplacement des endroits qui accèdent à un simili-live dans le backend
  • exposition de workingVersion au frontend
  • remplacement des appels à live
  • suppression des références à autosave
  • suppression des références à version dans les requêtes GraphQL (nouvel utilisateur, nouvel article)
  • déterminer la requête MongoDB à effectuer pour migrer la dernière version/autosave en workingVersion
    • la autosave/plus récente est copiée dans workingVersion
    • cet item est supprimé
  • prendre en compte la valeur latest sur le service d'export utilisé par les scripts de @marviro
    • Il faudrait éventuellement faire évoluer les scripts pour permettre d'exporter soit à partir d'un identifiant de version, soit d'un identifiant d'article (notion de copie de travail "latest")
    • Valider avec @marviro les routes actuellement utilisées par le script /cgi-bin/exportArticle/exec.cgi
  • ❤️ on commence à mémoriser des choix utilisateurs (ouverture/fermeture des panneaux latéraux, mode d'édition des métadonnées)

fixes #412
fixes #383
fixes #133
fixes #467

@netlify
Copy link

netlify bot commented Nov 18, 2021

✔️ Deploy Preview for stylo-dev ready!

🔨 Explore the source changes: 90e3ccd

🔍 Inspect the deploy log: https://app.netlify.com/sites/stylo-dev/deploys/61a73e0a962a540008dc85b9

😎 Browse the preview: https://deploy-preview-458--stylo-dev.netlify.app/

@ggrossetie
Copy link
Collaborator

@oncletom je n'ai pas (encore) testé en local mais sur la preview (Netlify) j'obtiens :

index.2712c8ec.js:37 Uncaught TypeError: Cannot read properties of undefined (reading 'apply')
    at index.2712c8ec.js:37
    at yh (index.2712c8ec.js:37)
    at YD (index.2712c8ec.js:88)
    at index.2712c8ec.js:98

J'utilise Chrome 95.0.4638.69.

Sinon j'ai ajouté un script de migration dans 555266c (#458)
Il faudra le tester/valider sur stylo-dev quand on décidera de fusionner sur stylo-dev.

@ggrossetie
Copy link
Collaborator

Je reproduis l'erreur en local, je pense que le initialState ne peut pas être une fonction.
J'ai fais un changement pour initialiser le store différemment.

@thom4parisot
Copy link
Member Author

Je reproduis l'erreur en local, je pense que le initialState ne peut pas être une fonction. J'ai fais un changement pour initialiser le store différemment.

Ah, zut.

Pourtant la signature de https://redux.js.org/api/createstore le mentionne. C'était notre paramètre undefined d'avant.
J'ai peut-être pas vérifié un cas de figure (genre sans localStorage initial, y'a peut-être un bug…)

@ggrossetie
Copy link
Collaborator

Pourtant la signature de redux.js.org/api/createstore le mentionne. C'était notre paramètre undefined d'avant.

Peut être qu'on est dans ce cas: "If you produced reducer with combineReducers, this must be a plain object with the same shape as the keys passed to it"

J'ai peut-être pas vérifié un cas de figure (genre sans localStorage initial, y'a peut-être un bug…)

Effectivement la fonction loadStateFrom... n'avait pas de return quand le localStorage est "vide" mais ça ne corrige pas le problème.
J'ai aussi eu cette erreur avec le compose donc je suis revenu à la signature initiale en ajoutant persistStateIntoLocalStorage dans la fonction applyMiddleware. Il y a peut être plus élégant mais avec cette incantation ça fonctionne 😉

@ggrossetie
Copy link
Collaborator

À noter qu'on corrige #412

@ggrossetie ggrossetie marked this pull request as ready for review November 24, 2021 14:05
@ggrossetie
Copy link
Collaborator

ça me semble pas mal, vu l'étendu des changements je pense qu'il faudrait pas laisser traîner cette pull request trop longtemps et prévoir un déploiement sur https://stylo-dev.huma-num.fr cette semaine ou semaine prochaine.

@thom4parisot
Copy link
Member Author

@Mogztter top ! J'ai voulu tester sur https://deploy-preview-458--stylo-dev.netlify.app/ mais y'a l'erreur comme quoi Redux est initialisé avec plusieurs store enhancers.

@ggrossetie
Copy link
Collaborator

J'ai voulu tester sur deploy-preview-458--stylo-dev.netlify.app mais y'a l'erreur comme quoi Redux est initialisé avec plusieurs store enhancers.

Je ne vois pas cette erreur, soucis de cache ?

@ggrossetie
Copy link
Collaborator

Pour le déploiement, le script de migration peut être lancé plusieurs fois mais comme on supprime des données, il faudrait faire un backup avant de le lancer afin de pouvoir revenir en arrière si besoin.

La procédure de backup est détaillée ici: https://github.com/EcrituresNumeriques/stylo/blob/master/SERVER.md#backup

@ggrossetie
Copy link
Collaborator

mais y'a l'erreur comme quoi Redux est initialisé avec plusieurs store enhancers.

Ou alors c'est peut être quand les React DevTools sont actifs ? Bref, chez moi ça fonctionne toujours avec tes derniers changements donc 👍🏻

@thom4parisot
Copy link
Member Author

Oui c'est ça, ça déconnait avec les Redux DevTools (en plus des React DevTools).
bfddbff résout ça.

Thomas Parisot added 4 commits December 1, 2021 11:01
It moves the defaults to the article creation, because a version is always based on a `workingVersion` of an article. So no defaults needed.
Thomas Parisot and others added 25 commits December 1, 2021 11:01
Co-authored-by: Thomas Parisot <oncletom@users.noreply.github.com>
Co-authored-by: Thomas Parisot <oncletom@users.noreply.github.com>
Co-authored-by: Thomas Parisot <oncletom@users.noreply.github.com>
Co-authored-by: Thomas Parisot <oncletom@users.noreply.github.com>
@ggrossetie ggrossetie merged commit e3de56a into master Dec 1, 2021
@ggrossetie ggrossetie deleted the current-version branch December 1, 2021 10:02
thom4parisot added a commit that referenced this pull request Jan 7, 2023
It also solves a regression introduction in #458, specifically in e3de56a

fixes #639

fixes #688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants