-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: dev
Are you sure you want to change the base?
Conversation
d827cdf
to
0920cbd
Compare
module["{{ call.function }}"]( | ||
attempt, | ||
{% if call.data %} | ||
JSON.parse(htmlDecode("{{ call.data }}")) |
There was a problem hiding this comment.
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."
reqAttempt(["{{ call.module }}"], function(module) { | ||
module["{{ call.function }}"]( |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
<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> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Ich habe starke Bauchschmerzen in einem neuen Projekt auf AMD/RequireJS zu setzen.
Verschiedene Versionen innerhalb einer Seite zu laden ist grundsätzlich auch mit anderen Mitteln möglich: Dynamische Imports, Shadow DOM, Bundler.
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).
Multiple import maps kommen. Bis es soweit ist, müssen wir eben einen Workaround anbieten:
Argumente gegen RequireJS
Links
|
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.
Ausgerechnet "Provide a programmatic way to expand or modify the Window’s import map" ist ein non-goal. :/
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 Ich erstelle in Tefen gerade eine Übersicht, in der ich versuche die Einbindungsarten (direkt in DOM, Shadow DOM, iframe) zu vergleichen. |
Edit: Recherche zu Modulformat und -lader Original postWollte 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.
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?
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.
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).
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: SystemJSIch 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. |
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.""" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* @param {Element} formulationElement | ||
* @param {Element} generalFeedbackElement | ||
* @param {Element} specificFeedbackElement | ||
* @param {Element} rightAnswer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sind die nicht Nullable?
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 werdenpaths
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.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