search

alvarcarto / url-to-pdf-api

Web page PDF/PNG rendering done right. Self-hosted service for rendering receipts, invoices, or any content.
MIT License
7.03k stars 779 forks source link

Upgrade puppeteer to 1.14 #103

Closed kjagiello closed 5 years ago

kjagiello commented 5 years ago

I have experienced some issues with canvas rendering on some platforms and it seems that it was resolved in newer versions of puppeteer, which were not supported by this library. This PR updates puppeteer to the latest version while making sure that all the tests are still passing.

The motivation behind the switch from pdf2json to pdf-parse was that the former was not able to correctly read the PDF:s generated by the updated Chromium version. Looking at the GitHub issues of that project showed that it was quite buggy and not really maintained anymore. pdf-parse on the other hand had no problem with PDF parsing, so the choice was easy.

kjagiello commented 5 years ago

I see that some tests are failing. The first two are simply timing out because of the low timeout set on the test suite (https://github.com/alvarcarto/url-to-pdf-api/blob/master/test/test-all.js#L34). I have however no explaination for why the assertion fails, as you can clearly see that the text it is looking for is actually present in the PDF text. I will investigate and come back to you will a fix.

kjagiello commented 5 years ago

Looks like the cookie issue is caused by text exported from the cookie PDF being different on Mac vs Linux.

# Linux
00000000: 636f 6f6b 6965 7320 7265 6365 6976 6564  cookies received
00000010: 3a0a            

# Mac                         :.
00000000: 636f 6f6b 6965 73c2 a072 6563 6569 7665  cookies..receive
00000010: 643a 0a                                  d:.

The difference is basically that we get a regular space on Linux (0x20) and a non-breaking space on Mac (0xC2 0xA0). Not sure if the issue is caused by Skia generating PDFs differently depending on the platform or if it's the same issue, but with pdf-parse.

kjagiello commented 5 years ago

@kimmobrunfeldt do you think we could get this merged in?

alanfriedman commented 5 years ago

Oops, I didn't notice this PR and looks like I made the same fix in #105! Would be great to get this merged and I can just have my PR be for dotenv

kjagiello commented 5 years ago

Oops, I didn't notice this PR and looks like I made the same fix in #105!

Great minds think alike they say haha!