openzim / gutenberg

Scraper for downloading the entire ebooks repository of project Gutenberg
https://download.kiwix.org/zim/gutenberg
GNU General Public License v3.0
126 stars 37 forks source link

Remove unused variables from tool.js #173

Closed theparthshukla closed 1 year ago

theparthshukla commented 1 year ago

fix #158 Removed unused variables, however left unused functions intact.

benoit74 commented 1 year ago

LGTM from my mobile, I will run a more detailed review (including a test with a real ZIM) later to confirm this.

@rgaudin @kelson42 @theparthshukla : do you have experience of situations where old browsers would complain that JavaScript function are missing an argument?

Here an additional argument is passed by the caller but not used in the function, so we can remove it, but is it a recent convention or something which has almost always been supported in JavaScript ?

I'm a bit afraid of testing these JavaScript on recent modern browsers while we know that a significant portion of Kiwix/ZIM users are still on very old versions

kelson42 commented 1 year ago

This is more a question for our JS expert @Jaifroid

Jaifroid commented 1 year ago

@benoit74 Gutenberg ZIMs can be read on old browsers so long as the user uses the native search to access book covers and the books themselves, and not the proprietary UI (which doesn't work in Kiwix JS versions if the browser doesn't support Service Workers, because only in Service Worker mode can the app run JS in the ZIM).

There is, however, a specific issue #145 with Chromium extensions (Chrome, Edge, etc.). These ban the use of inline JS, which means that the proprietary UI also doesn't work in the latest Chromium browsers in the extension/add-on version. The UI does work in the PWA version (pwa.kiwix.org).

Generally it's pretty trivial to change the use of inline JS by putting inline scripts into a separate .js file and simply loading it in the HTML.

rgaudin commented 1 year ago

Here an additional argument is passed by the caller but not used in the function, so we can remove it, but is it a recent convention or something which has almost always been supported in JavaScript ?

No it's been like this for as long as I can remember. JS is very flexible regarding this.

kelson42 commented 1 year ago

What is the status here?

benoit74 commented 1 year ago

Never had time to review this in more details, and it looks like it will not happen in the short term :-(

Le mer. 8 mars 2023 à 06:51, Kelson @.***> a écrit :

What is the status here?

— Reply to this email directly, view it on GitHub https://github.com/openzim/gutenberg/pull/173#issuecomment-1459580387, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWF5CJ23M27QGSNQPIDIYDW3AM4HANCNFSM6AAAAAAVE4T42U . You are receiving this because you were mentioned.Message ID: @.***>

kelson42 commented 1 year ago

@theparthshukla Any news?

theparthshukla commented 1 year ago

@kelson42 whoops, the notification seems to have missed my mailbox. Will get to this later today

theparthshukla commented 1 year ago

Line #65 ‘goToAuthor’ is defined but not used. However, it is actually used in “cover_article.html” on line #20.

Line #392 'showBookshelf' is defined but never used. However, it is actually used in “base.html” on line #29.

Line #611 ‘init’ is defined but never used. However, it is actually used in “base.html” on line #29.

These were 3 issues of the 28 that were not taken care of as they were incorrectly identified.

Upon removal of the function “queryParams” as it was a function made redundant by the removal of the function “getRequestedPage”, (not identified in the initial issues) we get a total of 26 issues fixed.