search

pofider / phantom-html-to-pdf

Highly scalable html to pdf conversion using phantom workers
MIT License
159 stars 33 forks source link

reset zoomFactor after page render #42

Closed ghost closed 7 years ago

ghost commented 7 years ago

After some more testing with my previous changes, it seems I introduced a bug. When setting the zoomFactor it needs to be reset after the page render, otherwise it will effect new pages as them come in, causing elements to be cut off.

pofider commented 7 years ago

I don't get this. I thought page object is unique instance for the whole rendering. Could you give me an example html so I can see this?

ghost commented 7 years ago

Sure. I created a test application with some dummy html to try and simulate how we are using this library.

https://github.com/ianloverink/pdf-test

The master branch is using the 0.4.8 tagged release The test-changes branch is using my fork with the changes in the PR.

To use just node index.js and hit localhost:8080/convert. It will take the contents of test.html and write out to test.pdf. Using the master branch if you hit the url multiple times, the zoom will be inconsistent and elements will get cut off the page. Using the test-changes branch, after a clean npm install, hitting the url multiple times results in a consistent zoom and no elements getting cut off.

pofider commented 7 years ago

Great. Thank you for very much for reproducing the repository.

Now I see the problem - we are not creating new phantomjs Page object for each request but keep just one global. This causes the zoomFactor to be altered persistently and your sizing calculations are then based on the previous zoomFactor.

I was doing some performance tests and there is real reason for reusing the page object, because phantomjs is not releasing memory for it. Everything is fine as long as we set all the page properties at the beginning of the request and don't queue multiple requests into phantomjs at once.

Were you're solution almost works, I would propose to make a little adaptation. Lets set always default zoomFactory to 1 unless there is fitToPage param. This makes it working also if there was an exception thrown during previous render and it is more obvious. https://github.com/pofider/phantom-html-to-pdf/blob/master/lib/scripts/conversionScriptPart.js#L123

Could you do this change so I can't merge it?

ghost commented 7 years ago

Updated. It will now set the page.zoomFactor to 1 always and if fitToPage is set it will execute the zoom logic.

pofider commented 7 years ago

Great, thank you.

pofider commented 7 years ago

Released in 0.4.9