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

JavaScript in Attempts ermöglichen #133

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

MartinGauk
Copy link
Contributor

@MartinGauk MartinGauk commented Oct 29, 2024

  • Innerhalb des Attempts kann das Paket per call_js(module, function, data, if_role_feedback) veranlassen, dass das LMS im Browser eine JS-Funktion aufrufen soll.
  • module kann aktuell nur als volle Referenz in der Form @[package namespace]/[package short name]/[subdir]/[module name] angegeben werden
  • Das static-files example wird erweitert um ein simples JS-Beispiel.
  • JS wird mittels RequireJS geladen, d.h. das JS muss vom Paket als AMD-Modul bereitgestellt werden. Auch wenn RequireJS als out gilt, bietet es gerade in unserem Fall große Vorteile:
    • Dank des Multiversion Supports können wir RequireJS auf einer Seite mehrfach instanziieren und auf diese Weise problemlos mehrere Fragen anzeigen lassen, auch wenn die einzelnen Fragen Abhängigkeiten zu unterschiedlichen Versionen haben sollten.
    • Durch die paths Einstellung können wir sehr leicht bestimmen, welches Modul von wo geladen werden soll und das Laden aus den Attempt files ermöglichen und Paket-Hashes in die URL einbauen.
    • ES6 Module und importmaps sind für uns nicht so gut machbar, da auf einer Seite nur eine importmap erlaubt ist und im Zusammenspiel mit dem LMS das irgendwann zu einem Problem führen könnte bzw. das Anzeigen mehrerer Fragen erschwert.

Abhängig von questionpy-org/questionpy-server#120

module["{{ call.function }}"](
attempt,
{% if call.data %}
JSON.parse(htmlDecode("{{ call.data }}"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| tojson wäre hier einfacher und laut Doku auch "safe to render in HTML documents and <script> tags."

Comment on lines +174 to +175
reqAttempt(["{{ call.module }}"], function(module) {
module["{{ call.function }}"](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auf den ersten Blick scheint das HTML-Autoescaping von Jinja zwar zumindest naive Injection ("\"böserCode()") zu verhindern, verändert dabei aber die Strings auch inhaltlich. Das ist vermutlich praktisch selten ein Problem, wir können aber recht simpel | tojson verwenden, da kommt dann ein korrekt escapter JS(ON)-String raus.

if_role_feedback: Function is only called if the user has this role or is allowed to view this
feedback type. If None, the function is always called.
"""
data_json = "" if data is None else json.dumps(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Keine Daten" würde ich lieber auch im Model mit None darstellen als mit dem leeren String, auch wenn das vermutlich nur stilistisch ist.

Comment on lines +8 to +10
<script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.7/require.js"
integrity="sha512-H/RK9lhgLZE7IvypfHj5iUX0fnbaz5gA8y81NQ8F6azabccQuFAVeQdvOYDeAvAsl/WZTOGphkwhhlpCJi157A=="
crossorigin="anonymous" referrerpolicy="no-referrer"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaScript von externen Anbietern einzubinden halte ich für problematisch und unnötig.

  • Datenschutz: CDNs können Verbindungen unkontrolliert an Staaten außerhalb der EU weiterleiten. Das ist ein Datenschutzproblem und ist im Zweifel nicht DSGVO-konform.
  • Setzt aktive Internet-Verbindung vorraus (z.B. jemand der im Zug arbeitet o.ä. ist außen vor).
  • Security: Legt die Verantworung effektiv in die Hand eines externen Anbieters. Kompromittierte CDNs hat es schon gegeben...
  • Downtimes: Es hat in der Vergangenheit CDN-Downtimes gegeben.

Im Allgmeinen auch unnötig, da es Alternativen gibt:

  • RequireJS gar nicht erst nutzen (siehe anderen Kommentar)
  • Die JS-Datei direkt ins Projekt schreiben (wäre hier m.M.n. die pragmatische Lösung, wenn an RequireJS festgehalten wird)
  • (NPM verwenden) - kommt ja wahrscheinlich eh bald mit dem neuen Frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich bin auch kein Fan von CDNs, aber vor dem Hintergrund der angehenden kompletten JS-Umstrukturierung ist das eine pragmatische Zwischenlösung. Später sollte das beim Bundlen direkt mitgeliefert werden.

Bezüglich RequireJS habe ich eingangs mehrere Gründe genannt. Ich denke es bietet eine große Flexibilität für dynamische Abhängigkeiten. Dagegen sind ES6 und die Importmap leider viel zu statisch und andere moderne Alternativen für genau unseren Anwendungsfall mit den Fragepaketen sehe ich nicht.

@tumidi
Copy link
Contributor

tumidi commented Nov 12, 2024

Ich habe starke Bauchschmerzen in einem neuen Projekt auf AMD/RequireJS zu setzen.

Dank des Multiversion Supports können wir RequireJS auf einer Seite mehrfach instanziieren und auf diese Weise problemlos mehrere Fragen anzeigen lassen, auch wenn die einzelnen Fragen Abhängigkeiten zu unterschiedlichen Versionen haben sollten.

Verschiedene Versionen innerhalb einer Seite zu laden ist grundsätzlich auch mit anderen Mitteln möglich: Dynamische Imports, Shadow DOM, Bundler.

Durch die paths Einstellung können wir sehr leicht bestimmen, welches Modul von wo geladen werden soll und das Laden aus den Attempt files ermöglichen und Paket-Hashes in die URL einbauen.

Am Ende des Tages kommen wir an einem Indirektionsschritt nicht vorbei, wenn wir ein "custom" Modulschema anbieten möchten, seien es importmaps, die Verwendung eines Bundlers oder eine andere Technik (wobei importmaps als natives Browserfeature das Mittel der Wahl wären).

ES6 Module und importmaps sind für uns nicht so gut machbar, da auf einer Seite nur eine importmap erlaubt ist und im Zusammenspiel mit dem LMS das irgendwann zu einem Problem führen könnte bzw. das Anzeigen mehrerer Fragen erschwert.

Multiple import maps kommen. Bis es soweit ist, müssen wir eben einen Workaround anbieten:

  1. Die eines Nutzung Bundler voraussetzen bzw. vereinfachen (bspw. durch ein Projekt-Template und gute Doku),
  2. Für Pakete ohne Build-Step, einen Wrapper qpyImport() anbieten, der das Mapping transparent erledigt, o.ä.
  3. Einen Endpoint anbieten, der die gewünschten Informationen in die URL einbettet und die richtige Datei ausliefert.
    • Ich denke, das könnte mittels eines ServiceWorkers sogar transparent geschehen. (mehr Research nötig)
  4. Der Vollständigkeit halber gäbe es auch die Möglichkeit, eine importmap im Backend zu mergen. Allerdings würde diese Lösung, Support im LMS/Application-Server voraussetzen (was bei Moodle aktuell nicht der Fall ist).

Argumente gegen RequireJS

  • RequireJS ist überholt.
  • RequireJS fügt zusätzliche Komplexität hinzu, ESM funktionieren nativ in Browsern.
  • Der Trend geht zu "pure ES"-Paketen.
    • Immer mehr Libraries bieten ausschließlich ESM-Builds an.
    • QuestionPy-Pakete müssten eh einen Bundler o.ä. verwenden, um diese Projekte in RequireJS zu verwenden (was die Verwendung von RequireJS überflüssig macht, da Bundler eigene Modullader mitbringen).
  • Entwickler*innen müssten ein veraltetes Modulsystem lernen, bspw. define-Semantik und -Syntax.
  • Tree shaking ist nur mit ESM möglich.

Links

@MartinGauk
Copy link
Contributor Author

Grundsätzlich hast du in einigen Argumenten völlig recht, allerdings spielen in der Diskussion Tree Shaking, veraltetes Modulsystem lernen und Bundling keine Rolle. Das geschieht beim Bauen eines einzelnen Pakets. Es geht an der Stelle ausschließlich um die Fähigkeiten, im Client JS zu laden und die Abhängigkeiten zu verwalten. Und das sind Punkte, in denen RequireJS die flexibelsten Funktionalitäten bietet.

Multiple import maps kommen. Bis es soweit ist, müssen wir eben einen Workaround anbieten:

Ausgerechnet "Provide a programmatic way to expand or modify the Window’s import map" ist ein non-goal. :/

  • QuestionPy-Pakete müssten eh einen Bundler o.ä. verwenden, um diese Projekte in RequireJS zu verwenden (was die Verwendung von RequireJS überflüssig macht, da Bundler eigene Modullader mitbringen).

Ich will RequireJS nicht als Bundler nutzen sondern nur als Lader auf der Client-Seite. Die Pakete sollen dynamische Abhängigkeiten zu anderen Paketen haben dürfen, womit auch das JS der anderen Pakete geladen können werden darf. Dem Bundler wird beim Paketbauen gesagt, dass es sich um eine externe Abhängigkeit handelt, d.h. auf der Client-Seite muss es dann einen Modullader geben.


Für den Aufruf einer JS-Funktion hatte ich zwischenzeitlich überlegt, dass ein Objekt von Attempt nicht als erstes Argument übergeben wird, sondern man das Objekt in sein Modul importiert. Dazu muss pro Frage ein dynamisches Modul erstellt werden, in dem sich das Objekt befindet. Das ist mit RequireJS problemlos möglich, weil man eine RequireJS-Instanz pro Frage erstellen kann und ein Modul nicht in einer einzelnen/realen Datei stecken muss. Bei ESM sehe ich dafür keine Chance.


Ich erstelle in Tefen gerade eine Übersicht, in der ich versuche die Einbindungsarten (direkt in DOM, Shadow DOM, iframe) zu vergleichen.

@tumidi
Copy link
Contributor

tumidi commented Nov 15, 2024

Edit: Recherche zu Modulformat und -lader

Original post

Wollte hier meine aktuellen Gedanken/Notizen festhalten. Wir können aber gern auch auf dem nächsten Meeting in Person darüber reden, wenn das einfacher ist.

allerdings spielen in der Diskussion Tree Shaking, veraltetes Modulsystem lernen und Bundling keine Rolle. Das geschieht beim Bauen eines einzelnen Pakets.

Ich denke, diese Punkte werden für frontend-lastige QPy-Pakete potentiell durchaus eine Rolle spielen.

Gehst du von einem (impliziten) Transpilierungsschritt während des Paketbauens aus? Also, dass das SDK während des Bauens automatisch und transparent einen Bundler o.ä. verwendet?

Es geht an der Stelle ausschließlich um die Fähigkeiten, im Client JS zu laden und die Abhängigkeiten zu verwalten.

Die Themen sind verflechtet und lassen sich kaum unabhängig betrachten. Eine Entscheidung auf der einen Seite (JS im Client laden) hat ja Einfluss auf die andere Seite (in welchem Format JS-Quellen in QPy-Paketen vorliegen müssen), und andersherum.

Und das sind Punkte, in denen RequireJS die flexibelsten Funktionalitäten bietet.

Ich denke halt, RequireJS wäre (durch die Begrenzung auf AMD) weniger flexibel als ESM. QPy-Pakete wären effektiv dazu gezwungen eine JS-Build-Toolchain einzusetzen (siehe nächster Punkt).

  • QuestionPy-Pakete müssten eh einen Bundler o.ä. verwenden, um diese Projekte in RequireJS zu verwenden (was die Verwendung von RequireJS überflüssig macht, da Bundler eigene Modullader mitbringen).

Ich will RequireJS nicht als Bundler nutzen sondern nur als Lader auf der Client-Seite. Die Pakete sollen dynamische Abhängigkeiten zu anderen Paketen haben dürfen, womit auch das JS der anderen Pakete geladen können werden darf. Dem Bundler wird beim Paketbauen gesagt, dass es sich um eine externe Abhängigkeit handelt, d.h. auf der Client-Seite muss es dann einen Modullader geben.

Vielleicht habe ich das schlecht ausgedrückt oder wir missverstehen uns.

RequireJS kann keine ES-Module verarbeiten, was problematisch ist, wenn wir es als Modullader voraussetzen.

Wenn QuestionPy die Verwendung von RequireJS/AMD erzwingt, hat das Konsequenzen. Beispiel: Die aktuelle Version von XY liefert kein UMD-Build mehr mit. Möchte nun ein QPy-Paket XY nutzen wäre die einzige Option, ein eigenes UMD-Build mit Babel oder einem Bundler wie Webpack zu erstellen.

Das aber setzt Wissen über Modulsysteme und JS-Tooling voraus, und nicht alle Projekte lassen sich problemlos in UMD umwandeln, da die beiden Modulssysteme unterschiedliche Semantiken haben. Dieses Problem wird noch zunehmen, da immer weniger Projekte UMD-Builds anbieten.

Alternative: SystemJS

Ich denke, wenn wir einen Modullader clientseitig einsetzen, sollten wir uns SystemJS als Alternative zu RequireJS anschauen.

Edit: Ein weiterer Punkt ist ja die Verwendung von Moodle (bzw. allgemein die Verwendung von LMS-JS-Komponenten, wie dem Filepicker oder Editor). Ich denke, SystemJS wäre hier flexibel genug, die entsprechenden Objekte "einzusammeln" und den Fragen unter einem "virtuellen" Modulnamen anzubieten.

Comment on lines +172 to +174
self._javascript_calls: dict[JsModuleCall, None] = {}
"""LMS has to call these JS modules/functions. A dict is used as a set to avoid duplicates and to preserve
the insertion order."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum möchten wir keine Duplikate erlauben?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Habe ich in Tefen unter 5. beschrieben.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, damit schränken wir aber allgemein die Funktionalität ein, um ein spezifisches Problem zu lösen. Es gibt sicher auch Funktionen, die mehrmals aufgerufen werden müssen, oder wo bei mehreren Aufrufen der letzte behalten werden sollte und nicht der erste. Jedenfalls ist es für die Paketentwickler*innen so nicht intuitiv.

Vielleicht kannst du eine zweite Funktion call_js_once hinzufügen, die keine Duplikate erlaubt, oder einen Parameter call_only_once?

Copy link
Contributor Author

@MartinGauk MartinGauk Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oder wo bei mehreren Aufrufen der letzte behalten werden sollte und nicht der erste

Die Duplikate beziehen sich auf module, function, data und if_role/feedback_type. Oder warst du davon ausgegangen, dass nur nach Modul/Funktion gegangen wird?

Der Use-Case wird mir nicht klar, warum man eine Funktion mit denselben Daten mehrmals aufrufen sollte. Also ich glaube das ist eher ein unerwünschtes Verhalten, zu dem es aber schnell kommen kann, wenn man mehrere gleichartige Subquestions hat.

Wenn das Verhalten bleibt, sollte dies aber in jedem Fall noch in der call_js Methode dokumentiert werden.

module: JS module name specified as:
@[package namespace]/[package short name]/[subdir]/[module name] (full reference) or
TODO [subdir]/[module name] (referencing a module within the package where this class is subclassed) or
TODO attempt/[module name] (referencing a dynamically created module returned as an attempt file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Können wir irgendwie mögliche Kollisionen mit einem [subdir] namens "attempt" vermeiden?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventuell ist auch die Frage, ob wir diesen Fall (attempt files) wirklich unterstützen wollen/müssen. Es gibt ja außerdem noch andere Bereiche, in denen das Paket Dateien ablegen kann.
Für diese Nicht-Standard-Fälle könnten wir auch QPy-URIs vorschreiben. Initial würde ich das aber eh noch nicht implementieren und können wir später noch genauer überlegen.

Comment on lines +26 to +29
* @param {Element} formulationElement
* @param {Element} generalFeedbackElement
* @param {Element} specificFeedbackElement
* @param {Element} rightAnswer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sind die nicht Nullable?

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

Successfully merging this pull request may close these issues.

4 participants