search

assemblee-virtuelle / archipelago

Fostering interconnections between communities by creating synergies between their platforms
Apache License 2.0
14 stars 6 forks source link

DereferenceMixin: utiliser ldp.resource.get au lieu de ldpNavigator #158

Closed srosset81 closed 4 months ago

srosset81 commented 6 months ago

Dans la PR https://github.com/assemblee-virtuelle/archipelago/pull/157, j'ai été obligé de désactiver à nouveau le disassembly car le code du DereferenceMixin ne fonctionne plus avec la dernière version de SemApps (il n'y a plus de defaultContext exporté de @semapps/core)

De toute façon, je suis dubitatif quant aux lignes de codes suivantes qui changent le context arbitrairement. Cela ne me semble pas durable.

https://github.com/assemblee-virtuelle/archipelago/blob/next/middleware/mixins/dereference.js#L28-L35

Si ldpNavigator ne peut pas gérer correctement un context local, ce serait beaucoup plus simple d'appeler l'action ldp.resource.get qui, lui, le gère très bien (et avec du cache pour éviter d'aller fetcher le context à chaque fois)

simonLouvet commented 6 months ago

Ce qui me dérange, c'est que pour pouvoir faire un mixin paramétrable il faut passer un plan de déréférencement et que ldpNavigator a déjà un algo de déréférencement et je ne trouve pas cela optimisé de le réécrire copier/coller.

Il est bien sur facile de faire un controlledContainer qui fait du déréférencement par ldp.resource.get manuellement, mais je trouve cela dommage.

J'ai réfléchi à une solution qui répondrait au deux besoin. réaliser un semappsAdapter plutôt qu'utiliser un fetchAdapter . Le semapps adapter aurait le ctx moleculer en paramètre et pourrait faire les ldp.resource.get en suivant le dereferencPlan. C'est de l'énergie dépensée alors qu'on doit migrer le déréférencement sur le front... Pour le context modifié avant et après le déréférencement, il y a une raison, et c'est documenté dans le code : la librarie jsonld necessite des context publié sur le web et non localhost ce qui était le cas dans nos test. Il n'y en aura plus besoin en passant par ldp.resource.get directement ou à travers un adapter ldpNavigator.

srosset81 commented 6 months ago

Pour le context modifié avant et après le déréférencement, il y a une raison, et c'est documenté dans le code : la librarie jsonld necessite des context publié sur le web et non localhost ce qui était le cas dans nos test. Il n'y en aura plus besoin en passant par ldp.resource.get directement ou à travers un adapter ldpNavigator.

Oui j'ai bien compris que c'était pour ça. C'est pour cette raison qu'on a un JsonLdService qui met en cache le context local. Mais le code du DereferenceMixin est mauvais car il part de l'idée qu'on utilise toujours le context local, ce qui n'est pas le cas. Et surtout ça ne marche plus avec le refacto qui a été réalisé pour rendre le context plus flexible.

Faire le déréférencement en appelant ldp.resource.get est l'affaire de quelques lignes de code (un peu prise de tête certes), et au moins après on a quelque chose de clean qui pourrait même être publié comme mixin dans le package @semapps/ldp. Je ne vois pas en quoi ça simplifierait de créer un "semappsAdapter". Il n'y a vraiment aucun besoin de ça, en tout cas pour Archipelago.

simonLouvet commented 6 months ago

créer un semappsAdapter permet de réutiliser le code qui exécute de dereferencePlan qui existe déjà dans ldpNavigator pour éviter de le copier/coller dans DerefrenceMixin. Je comprends que ça te parait lourd pour répondre au besoin 'simple' de archipelago et je suis assez d'accord. Je cherche qqc de plus léger.