Enhance Error Handling in ZIMArchive and ZIMArchiveLoader | issue #813 #1273

ombalgude · 2024-07-22T13:36:06Z

We have successfully refactored ZIMArchive and ZIMArchiveLoader to eliminate UI dependencies, thereby enhancing backend adaptability. Instead of using direct UI alerts, we have implemented callback-based error handling, which enables more efficient error management. Furthermore, we have updated methods to ensure improved error reporting and modularity. This modification increases compatibility with environments such as Web Workers and optimizes overall error management.

…or handling issue no 813

ombalgude · 2024-07-22T14:06:48Z

This is my first open-source contribution. I hope it will be helpful. I enjoyed contributing. If there are any changes, please suggest them.😄

audiodude

Thank you so much for the code contribution! I like the general idea of this code, and I look forward to reviewing it further but I got caught up because it looks like it may be broken. Please take a look and let me know.

Also, please refrain from adding unrelated style changes (whitespace including indentation, parameter spacing, single quotes -> double quotes, etc)

www/js/lib/zimArchive.js

ombalgude

@audiodude I've made the requested changes. Please review them when you have a moment.

Jaifroid · 2024-07-23T21:06:22Z

@ombalgude Thank you for your contribution! We appreciate your effort to improve the project. To help us better understand and review your changes, could you please provide some additional information?

~~Which issue is this PR addressing?~~ OK I see from the PR title that it is Remove the dependencies on the UI of js backend files #813, so normally you would link the PR to the issue either from the right-hand Development option, or by writing "Fixes [issue number]" in your first comment. If there isn't an existing issue, it would be helpful to create one explaining the rationale behind these changes.
Could you clarify which dependencies you were aiming to eliminate?
We noticed several changes without accompanying comments or explanations. It would be great if you could add some context.
You've made some stylistic changes (e.g., switching from single to double quotes), as @audiodude noted, which we do warn against doing in CONTRIBUTING, so kindly go through and eliminate purely stylistic changes, so we can see what is of substance in your PR.

For future contributions, here are some tips that can help streamline the process:

Discuss your proposed approach on the relevant issue before submitting a PR. This allows for valuable feedback early in the process.
For style-related changes, please refer to our CONTRIBUTING guide. Following these guidelines ensures consistency across the project. We have an ESLint definition that can ensure you stick to the house style if you use an IDE that supports it.
When requesting a review, it's helpful to include information about the testing you've performed.

If you have any questions about these suggestions, please don't hesitate to ask.

Jaifroid · 2024-07-23T21:12:30Z

@ombalgude I'm afraid tests are still failing. To diagnose the issue, please look at https://github.com/kiwix/kiwix-js/actions/runs/10064097481/job/27827615748?pr=1273 .

Jaifroid

Please see https://github.com/kiwix/kiwix-js/blob/main/CONTRIBUTING.md#testing and https://github.com/kiwix/kiwix-js/blob/main/TESTS.md to understand the automatic tests we do. The info should help you to work with the tests and debug your PR. Thanks.

ombalgude · 2024-07-26T06:59:32Z

Please see https://github.com/kiwix/kiwix-js/blob/main/CONTRIBUTING.md#testing and https://github.com/kiwix/kiwix-js/blob/main/TESTS.md to understand the automatic tests we do. The info should help you to work with the tests and debug your PR. Thanks.

@Jaifroid you please provide some guidance on the modifications required to successfully merge this PR? Any hints would be greatly appreciated.

Jaifroid · 2024-07-26T08:13:53Z

@Jaifroid you please provide some guidance on the modifications required to successfully merge this PR? Any hints would be greatly appreciated.

@ombalgude First of all I think you need to explain to us what this PR is aiming to do precisely. The corresponding issue had been fixed already, but it was reopened in 2022 because some unfixed dependency on uiUtil.js was found in dec_wrapper.js. I was slightly surprised to see your code doesn't touch dec_wrapper.js. It doesn't mean what you've done is invalid, but the main problem is that we have no idea what you actually changed in zimArchive.js or zimArchiveLoader.js due to the many many style changes. So, you need to start by reverting all the style changes. Until you do that we can't really help!

…provide some your valuable feedback

ombalgude · 2024-07-28T17:04:49Z

Hey @Jaifroid , I'm done with the new changes. Please check them out and give me some valuable feedback.

Jaifroid

Unfortunately, I don't see any difference with your latest commit. There are still a bunch of style and indentation changes that make the PR illegible. Please follow the previously given advice carefully, so we can understand what this PR attempts to do.

Jaifroid · 2024-07-28T17:08:33Z

CONTRIBUTING.md

It seems to me that this file has nothing to do with the PR, and shouldn't be part of it.

Jaifroid · 2024-07-28T17:09:09Z

TESTS.md

Ditto for this file. Kindly avoid changing files that are not directly related to the issue you are fixing.

Jaifroid · 2024-07-28T17:10:20Z

www/js/lib/zimArchive.js

+import zimfile from "./zimfile.js";
+import zimDirEntry from "./zimDirEntry.js";
+import util from "./util.js";
+import uiUtil from "./uiUtil.js";
+import utf8 from "./utf8.js";
+import translateUI from "./translateUI.js";


These are all stylistic changes and shouldn't be in the PR.

Jaifroid · 2024-07-28T17:12:08Z

www/js/lib/zimArchive.js

-        var fileArray = [].slice.call(fileList);
-        // The constructor has been called with an array of File/Blob parameter
-        createZimfile(fileArray);
+function ZIMArchive(storage, path, callbackReady, callbackError) {


What concrete code changes have you made to the ZIMArchive object definition? There are a mass of changes that are not explained, and it's impossible for me to see concretely what has changed in substance.

Jaifroid · 2024-07-28T17:13:45Z

www/js/lib/zimArchive.js

+        console.warn("Error setting archive listings: ", err);
+      });
+  });
+  {


Why have you added lines 260 to 387?

Jaifroid · 2024-07-28T17:14:50Z

www/js/lib/zimArchive.js

Changes in this file are illegible, and many are still style changes, or indentation changes.

Jaifroid · 2024-07-28T17:14:59Z

www/js/lib/zimArchiveLoader.js

Ditto for this file.

Jaifroid · 2024-07-28T17:20:08Z

@ombalgude Tests are still failing. You don't appear to have followed our advice to remove all stylistic changes. Most of your changes seem to be moving code around, but it's not clear what you've fixed.

I did suggest above that you please discuss and explain what your PR does in writing, and you have not done this.

Also, please update the branch from main (there is a button "Update branch" below, that should make this easy to do).

ombalgude · 2024-07-28T17:21:59Z

Ok @Jaifroid

Jaifroid · 2024-08-09T06:46:53Z

@ombalgude Do you intend to keep working on this PR, or shall I close it? Thanks.

Jaifroid · 2024-08-15T19:25:21Z

So I'm closing, but feel free to re-open if there are further contributions.

Refactor ZIMArchive methods to remove UI dependencies and improve err…

8ac127f

…or handling issue no 813

This comment was marked as resolved.

Sign in to view

audiodude requested changes Jul 23, 2024

View reviewed changes

www/js/lib/zimArchive.js Outdated Show resolved Hide resolved

Done with the changes please check it out

d705bf8

ombalgude commented Jul 23, 2024

View reviewed changes

ombalgude requested a review from audiodude July 23, 2024 18:35

Jaifroid self-requested a review July 23, 2024 20:54

Jaifroid requested changes Jul 24, 2024

View reviewed changes

i have completed the requested modifications. Kindly review them and …

b506b21

…provide some your valuable feedback

Jaifroid requested changes Jul 28, 2024

View reviewed changes

Jaifroid closed this Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Error Handling in ZIMArchive and ZIMArchiveLoader | issue #813 #1273

Enhance Error Handling in ZIMArchive and ZIMArchiveLoader | issue #813 #1273

ombalgude commented Jul 22, 2024

This comment was marked as resolved.

ombalgude commented Jul 22, 2024

audiodude left a comment

ombalgude left a comment •

edited

Loading

Jaifroid commented Jul 23, 2024 •

edited

Loading

Jaifroid commented Jul 23, 2024

Jaifroid left a comment

ombalgude commented Jul 26, 2024

Jaifroid commented Jul 26, 2024

ombalgude commented Jul 28, 2024

Jaifroid left a comment

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid Jul 28, 2024

Jaifroid commented Jul 28, 2024

ombalgude commented Jul 28, 2024

Jaifroid commented Aug 9, 2024

Jaifroid commented Aug 15, 2024

Enhance Error Handling in ZIMArchive and ZIMArchiveLoader | issue #813 #1273

Enhance Error Handling in ZIMArchive and ZIMArchiveLoader | issue #813 #1273

Conversation

ombalgude commented Jul 22, 2024

This comment was marked as resolved.

ombalgude commented Jul 22, 2024

audiodude left a comment

Choose a reason for hiding this comment

ombalgude left a comment • edited Loading

Choose a reason for hiding this comment

Jaifroid commented Jul 23, 2024 • edited Loading

Jaifroid commented Jul 23, 2024

Jaifroid left a comment

Choose a reason for hiding this comment

ombalgude commented Jul 26, 2024

Jaifroid commented Jul 26, 2024

ombalgude commented Jul 28, 2024

Jaifroid left a comment

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid Jul 28, 2024

Choose a reason for hiding this comment

Jaifroid commented Jul 28, 2024

ombalgude commented Jul 28, 2024

Jaifroid commented Aug 9, 2024

Jaifroid commented Aug 15, 2024

ombalgude left a comment •

edited

Loading

Jaifroid commented Jul 23, 2024 •

edited

Loading