Fix image loading issues by switching from MHTML to HTML format #71 #87
Conversation
Images hidden behind buttons were not displayed correctly when importing in MHTML. Sometimes, only the first image was displayed correctly, while the others remained white or were broken. To solve this problem, we switched to importing in HTML, which allows for better loading and display of images. This method ensures that all images are loaded and displayed correctly, improving the user experience.
There was a problem hiding this comment.
I have not tested the PR yet. Added a couple of comments. My main question is, do we really need the two packages (node-fetch and @types/node-fetch) you've added? Because you are calling fetch inside evaluate function's callback. Everything inside evaluate gets executed in the browser where we have access to fetch.
package.json
Outdated
| @@ -4,7 +4,9 @@ | |||
| "description": "Download course from educative.io", | |||
| "main": "src/app.ts", | |||
| "dependencies": { | |||
| "puppeteer": "^22.6.3" | |||
| "@types/node-fetch": "^2.6.12", | |||
There was a problem hiding this comment.
@types/node-fetch should be in devDependencies. I know it's a CLI tool and it does not really matter but, lets follow the standard.
package.json
Outdated
| @@ -4,7 +4,9 @@ | |||
| "description": "Download course from educative.io", | |||
| "main": "src/app.ts", | |||
| "dependencies": { | |||
| "puppeteer": "^22.6.3" | |||
| "@types/node-fetch": "^2.6.12", | |||
| "node-fetch": "^3.3.2", | |||
There was a problem hiding this comment.
Why do we need node-fetch? We are using Bun, and it has fetch API implemented to it.
_Note: NodeJS now has fetch API built into it.
src/browser.ts
Outdated
| if (srcset) { | ||
| const urls = srcset.split(',').map(url => url.trim().split(' ')[0]); | ||
| // Processes an array of URLs, fetching and converting those that start with '/api/' to data URLs. | ||
| const newSrcset = await Promise.all(urls.map(async url => { |
There was a problem hiding this comment.
Let's use Promise.allSettled instead of Promise.all.
|
Thank you for pointing this out! You are absolutely right that the Here are the changes I made:
|
| }); | ||
|
|
||
| // Wait a bit more for the new image URLs to load | ||
| await new Promise(resolve => setTimeout(resolve, 2000)); |
There was a problem hiding this comment.
Let's use the waitFor function that we have in the utils file.
| for (const img of Array.from(images)) { | ||
| const src = img.getAttribute('src'); | ||
| if (src?.startsWith('/api/')) { | ||
| const response = await fetch('https://www.educative.io' + src); |
There was a problem hiding this comment.
If you look at the configuration.ts file, we build urls in there. Can we introduce a new function over there and use the function to build the URL instead of hard-coding the root URL here?
Images were not displayed correctly when importing in MHTML. Sometimes, only the first image was displayed correctly, while the others remained grayed out.
To solve this problem, we switched to importing in HTML, which allows for better loading and display of images. This method ensures that all images are loaded and displayed correctly, improving the user experience.
Fixes #71