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

Fancy library doesn't work in browsers that do not support replaceAll #1174

Open
Jaifroid opened this issue Nov 27, 2023 · 5 comments
Open
Labels
bug-non-critical For bugs that it would be nice to fix rather than critical to fix dependencies Pull requests that update a dependency file upstream Issues that need to be dealt with at scraper level or some other repo user interface
Milestone

Comments

@Jaifroid
Copy link
Member

Jaifroid commented Nov 27, 2023

replaceAll() is used to construct the magnet link, and this throws an error in browser < Chrome/Edge 85 and Firefox 77. To be clear, this error only surfaces when you try to download a file. But as this is the whole point of the library, it really needs to be fixed...

See https://caniuse.com/?search=replaceAll.

We should probably polyfill that anyway, as the polyfill is available in core-js.

@Jaifroid Jaifroid added dependencies Pull requests that update a dependency file user interface bug-non-critical For bugs that it would be nice to fix rather than critical to fix labels Nov 27, 2023
@Jaifroid Jaifroid added this to the v4.0 milestone Nov 27, 2023
@Jaifroid
Copy link
Member Author

Jaifroid commented Nov 27, 2023

Actually, it's not yet in core-js 3. It's scheduled for 4 (a proposal).

We can get a polyfill like this:

npm install --save es6-string.prototype.replaceall

And where needed:

import 'es6-string.prototype.replaceall'; (although we probably need import '../../../node_modules/xxxx/dist/xxxx/xxxx.js';).

This would have to be added to the dependency grid. See https://github.com/kiwix/kiwix-js/blob/main/ADDING_DEPENDENCIES_NODE_MODULES.md for the method to do this.

@Jaifroid
Copy link
Member Author

@Rishabhg71 This is one that could interest you, as it could effectively fix your library feature for a wider range of browsers.

@Rishabhg71 Rishabhg71 self-assigned this Nov 27, 2023
@Greeshmanth1909
Copy link
Contributor

Greeshmanth1909 commented Jan 7, 2024

Will it be possible to "artificially" define a string.prototype.replaceAll method in the global scope of the program as a temporary fix for this issue until the release?

@Jaifroid
Copy link
Member Author

Jaifroid commented Jan 7, 2024

Will it be possible to "artificially" define a string.prototype.replaceAll method in the global scope of the program as a temporary fix for this issue until the release?

@Greeshmanth1909 I'll entertain any appropriate fix! We're probably a couple of weeks away from releasing v4.0, and it would certainly be good to get this fixed before that release. I don't think replaceAll should be too hard to emulate, and a full polyfill may be overkill.

@Jaifroid
Copy link
Member Author

@Greeshmanth1909 has discovered that this cannot be polyfilled in Kiwix JS due to CORS. However, it could be fixed upstream in kiwix-tools.

@Jaifroid Jaifroid added the upstream Issues that need to be dealt with at scraper level or some other repo label Jan 15, 2024
@Jaifroid Jaifroid modified the milestones: v4.0, v4.1 Feb 21, 2024
@Jaifroid Jaifroid modified the milestones: v4.1, v4.2 Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-non-critical For bugs that it would be nice to fix rather than critical to fix dependencies Pull requests that update a dependency file upstream Issues that need to be dealt with at scraper level or some other repo user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants