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

Added String.prototype.replaceAll method #1190

Closed
wants to merge 6 commits into from
Closed

Added String.prototype.replaceAll method #1190

wants to merge 6 commits into from

Conversation

Greeshmanth1909
Copy link
Contributor

resolves #1174
Added String.prototype.replaceAll method in case a browser doesn't support it by default.
Here are the tests I performed:

  • Ran npm run serve
  • Ran npm run preview
  • Tested with npm test
    In all the above cases I was able to browse and open zim files in both serviceWorker and JQuery modes in Chrome, Firefox (only JQuery) and Brave. I could't test the magnet link generation for older browsers.
  • Tested the extension for Firefox.
  • Tested the extension for Chrome in both JQuery and ServiceWorker modes.

@Jaifroid
Copy link
Member

Jaifroid commented Jan 9, 2024

@Greeshmanth1909 Thank you! I'll test this on a Chrome < 85 and Firefox < 77 with the download library.

@Jaifroid
Copy link
Member

I should get to this this weekend.

@Jaifroid Jaifroid mentioned this pull request Jan 12, 2024
39 tasks
@Greeshmanth1909
Copy link
Contributor Author

Pushed the code for a completely different issue to this pr by accident. Will fix it asap.

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Thanks very much for this PR. I tested it in Firefox Portable ESR 70.1, and unfortunately it doesn't appear to be working as intended. I get the error magnetLink.replaceAll is not a function when clicking on the "Download" link in the library. See screenshot below. Perhaps you haven't injected the function into the correct context? The library code runs in an iframe,

image

@Jaifroid
Copy link
Member

You can get different versions of Firefox Portable here: https://sourceforge.net/projects/portableapps/files/Mozilla%20Firefox%2C%20Portable%20Ed./

@Greeshmanth1909
Copy link
Contributor Author

Ill try fixing it. Thanks for the resource.

@Jaifroid
Copy link
Member

I was thinking you might run into CORS issues attempting to inject this function into the library iframe. In fact you almost definitely will, unless there is a known way to polyfill browser functions in a way that works also in iframes with third-party content. It seems unlikely, as it would be considered a vector for attack.

Given this, the only solution -- I think -- would be to make a PR (or suggest a PR) over at kiwix-tools (https://github.com/kiwix/kiwix-tools). I'm sorry if the issue misled you here, I only realized this might be an issue after polyfilling the function in init.js appears to have failed.

@Greeshmanth1909
Copy link
Contributor Author

I've also tried defining the polyfill in an inline script of library.html it didn't work.

@Greeshmanth1909
Copy link
Contributor Author

Shall I close this pr? @Jaifroid

@Jaifroid
Copy link
Member

Unfortunately, yes, it sounds like there is no solution on our end. But there is a solution on the kiwix-tools end, so I encourage you to open a ticket, and perhaps show the suggested polyfill code, upstream. Explain why it's needed -- to allow older browsers to use Kiwix Serve's download code.

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

Successfully merging this pull request may close these issues.

Fancy library doesn't work in browsers that do not support replaceAll
2 participants