Create and use a standard utility library for handling zip files in the frontend #11539

rtibbles · 2023-11-17T01:15:34Z

Summary

Generalizes usage of the fflate library in the H5P runner to create a kolibri-zip library for reading zip files in the browser
Allows users of the ZipFile utility to define Mapper classes that have getPaths and replacePaths methods, that then modify the file when it is fetched from the ZipFile
Mostly this is done to allow automatic recursing of the file to produce, for example, a CSS file with local file references (to fonts, images, etc) already replaced with a blob URL for a file that has been extracted from the zip file
This initial CSS use case used in the H5P Runner is then extended for src and href attributes in HTML and XML files
Lastly, the similar replacement done in JSON files in the Perseus Renderer is then updated to use the kolibri-zip library, with it defining its own custom JSONMapper for handle json files

References

The HTML and XML utilities were also created in conjunction with work on the draft PR #8070 (making the third use case for the abstraction)

Reviewer guidance

Is kolibri-zip a good enough name?

These changes affect, but in my manual testing do not alter the behaviour of both H5P file rendering and Perseus rendering. Some confirmation of this would be helpful.

It is worth noting that this work is likely to cause merge conflicts with #9759 - but it seemed simpler to do this change now, and resolve any conflicts in that PR.

The last issue that might need to be cleared up is the implicit dependency of a Hashi script on JSZip - this can be resolved within this PR, but is probably best left to a follow up issue, where we also update our bundled H5P static files.

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

Add support for xml and html file mapping - but only for src attributes.

github-actions · 2023-11-17T01:50:32Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b6.dev0_git.528.g3fc82593.pex
Windows Installer (EXE)	kolibri-0.16.0b6.dev0+git.528.g3fc82593-unsigned.exe
Debian Package	kolibri_0.16.0b6.dev0+git.528.g3fc82593-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b6.dev0+git.528.g3fc82593-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b6.dev0+git.528.g3fc82593-0.1.0-debug.apk
TAR file	kolibri-0.16.0b6.dev0+git.528.g3fc82593.tar.gz
WHL file	kolibri-0.16.0b6.dev0+git.528.g3fc82593-py2.py3-none-any.whl

nucleogenesis

The code and changes make sense throughout. I'm a little fuzzy on exactly how the get/replaceDomPaths functions are working w/ the regex. Maybe an example of what the mapping function is doing w/ the regex there could make it a bit more clear what is going on there?

nucleogenesis · 2023-11-22T19:56:35Z

packages/kolibri-zip/src/fileUtils.js

+      attributes
+        .map(a => element.getAttribute(a))
+        .filter(Boolean)
+        .map(url => url.replace(queryParamRegex, '$1'))


Does $1 here refer to the first captured match?

Yes - see here for the details: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace#specifying_a_string_as_the_replacement

marcellamaki · 2023-12-12T14:56:38Z

@nucleogenesis is there a blocker here? I'm not sure if I should review or if you are waiting for something specific. Can you clarify your recommendation a bit, just so we can keep this moving forward as it's gotten rather stale. Thank you!

marcellamaki

I have added a few in-line comments/questions for my own understanding, and will re-review after.

Manual QA of perseus and h5p checks out

marcellamaki · 2023-12-15T22:16:31Z

packages/hashi/src/H5P/H5PRunner.js

@@ -547,7 +523,7 @@ export default class H5PRunner {
      if (!file) {
        return;
      }
-      const json = JSON.parse(strFromU8(file.obj));
+      const json = JSON.parse(file.toString());


All of these file.toString() changes in this file are converting what was an object, to a string, which is the same behavior, but just in a different way than before, right?

Yes, the the toString method just uses the fflate strFromU8 method under the hood, this just allows the calling code to be agnostic about the underlying library.

marcellamaki · 2023-12-15T22:17:45Z

packages/hashi/src/H5P/loadBinary.js

@@ -20,7 +20,7 @@ export default function(path) {
        if (xhr.readyState === 4) {
          if (xhr.status === 200 || xhr.status === 0) {
            try {
-              resolve(new Uint8Array(xhr.response));
+              resolve(xhr.response);


Is this because the changes you've added mean that xhr.response is in a different format?

No - just because the loadBinary is only used for loading resources into the zip file, and I preferred to delegate the coercion to Uint8Array to the ZipFile class instead, and leave this utility to simply load the binary data.

marcellamaki · 2023-12-15T22:35:02Z

packages/kolibri-zip/src/fileUtils.js

+export function getAbsoluteFilePath(baseFilePath, relativeFilePath) {
+  // Construct a URL with a dummy base so that we can concatenate the
+  // dependency URL with the URL relative to the dependency
+  // and then read the pathname to get the new path.


I realize this was pre-existing and it's just moving it, so non-blocking.

This comment feels rather hard to wrap my brain around, mostly the
that we can concatenate the dependency URL with the URL relative to the dependency part
The second "URL" (i.e. "the URL relative to the dependency") is where I get lost.

Basically, we are using the JS URL utilities to resolve a relative file path (by turning it into a resolution of a relative URL against a dummy base URL).

So the file that we are currently looking at in the zip file: e.g. style/images.css references another file relatively ../images/cool.png, this will resolve the reference to an absolute file path from the root of the zip file - so images/cool.png.

This is a bit confusing for me as well but I played w/ it a little bit and I think that the thing that isn't clear is that the URL constructor auto-magically resolves the relative path against the second argument.

I played w/ it in the terminal a bit and this kind of made it make sense to me as I kept adding ..s to the relative path:

> new URL("../best/friend.css", "https://a.com/hi/there/my/mortal/enemy").pathname '/hi/there/my/best/friend.css' > new URL("../../best/friend.css", "https://a.com/hi/there/my/mortal/enemy").pathname '/hi/there/best/friend.css' > new URL("../../../best/friend.css", "https://a.com/hi/there/my/mortal/enemy").pathname '/hi/best/friend.css'

I think that maybe the comment could use some clarification or an example like the one you gave, Richard, re: styles/header/logo.css has a reference to ../../assets/logo.svg so we need to get a pathname assets/logo.svg out of that. Maybe that's more complicated though 😄

packages/kolibri-zip/src/index.js

marcellamaki · 2023-12-15T22:46:01Z

packages/kolibri-zip/generateMimeTypeDB.js

+  // Additional file types for Kolibri
+  'html',
+  'htm',
+  'xhtml',


I am not sure why these are added. Not that they shouldn't be, but more in the benefit/purpose way

As I note in the PR description:

The HTML and XML utilities were also created in conjunction with work on the draft PR #8070 (making the third use case for the abstraction)

I preferred to include these edits here, as it helps to triangulate the use case for the abstraction.

nucleogenesis

I left one comment but looking through the code again everything looks good. I don't need anything in particular for this to be merged although it may be worth updating the comment on getAbsolutePath to a JSDoc.

That's not blocking for me so as @marcellamaki plans to re-review I'll leave final approval to her

marcellamaki

Thanks @rtibbles - no blockers from me, but one remaining curiosity-only question

marcellamaki · 2024-01-02T20:30:22Z

kolibri/plugins/perseus_viewer/assets/src/views/PerseusRendererIndex.vue

+  Khan.imageUrls = {};
+
+  function getImagePaths(itemResponse) {
+    const graphieMatches = {};


What's graphie? Is this a perseus specific image term?

Yes - it allows loading of either a JSON blob or an SVG file to display graph style images.

rtibbles added 9 commits November 4, 2023 11:15

Create generic fflate wrapper utility.

95925d8

Simplify zip file loading to only need a URL.

2bdcd4b

Move blob url creation into kolibri-zip package.

00df0d2

Migrate file path substitution logic into kolibri-zip package.

2efb025

Extend mimetypes.

d559cc3

Create mapper class.

de14043

Add support for xml and html file mapping - but only for src attributes.

Add href substitution to xml and html unzipping.

9bdec4b

Add url caching and cleanup behaviour to kolibri-zip utility.

8336bbb

Update perseus renderer to use kolibri-zip functionality.

122d1e9

github-actions bot added the SIZE: large label Nov 17, 2023

rtibbles requested review from marcellamaki and nucleogenesis November 17, 2023 01:16

rtibbles added the TODO: needs review Waiting for review label Nov 17, 2023

rtibbles added this to the Kolibri 0.17: General maintenance, bugs, misc. important fixes milestone Nov 17, 2023

nucleogenesis reviewed Nov 22, 2023

View reviewed changes

marcellamaki reviewed Dec 15, 2023

View reviewed changes

nucleogenesis reviewed Dec 18, 2023

View reviewed changes

marcellamaki approved these changes Jan 2, 2024

View reviewed changes

rtibbles merged commit a855679 into learningequality:develop Jan 2, 2024
35 checks passed

rtibbles deleted the zippidy-doodah branch January 2, 2024 20:40

rtibbles mentioned this pull request Jan 2, 2024

Create a standard zip file reading utility wrapper using fflate #9157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create and use a standard utility library for handling zip files in the frontend #11539

Create and use a standard utility library for handling zip files in the frontend #11539

rtibbles commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

nucleogenesis left a comment

nucleogenesis Nov 22, 2023

rtibbles Dec 1, 2023

marcellamaki commented Dec 12, 2023

marcellamaki left a comment

marcellamaki Dec 15, 2023

rtibbles Dec 15, 2023

marcellamaki Dec 15, 2023

rtibbles Dec 15, 2023

marcellamaki Dec 15, 2023

rtibbles Dec 15, 2023

nucleogenesis Dec 18, 2023 •

edited

Loading

marcellamaki Dec 15, 2023

rtibbles Dec 15, 2023

nucleogenesis left a comment

marcellamaki left a comment

marcellamaki Jan 2, 2024

rtibbles Jan 2, 2024

Create and use a standard utility library for handling zip files in the frontend #11539

Create and use a standard utility library for handling zip files in the frontend #11539

Conversation

rtibbles commented Nov 17, 2023

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Nov 17, 2023

nucleogenesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellamaki commented Dec 12, 2023

marcellamaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis left a comment

Choose a reason for hiding this comment

marcellamaki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nucleogenesis Dec 18, 2023 •

edited

Loading