search

pofider / phantom-html-to-pdf

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

Add support for user defined cookies #55

Closed chips5k closed 7 years ago

chips5k commented 7 years ago

As discussed in issue https://github.com/pofider/phantom-html-to-pdf/issues/54

chips5k commented 7 years ago

Ah nice one, i was wanting to do something like that - avoid logging cookies in the main script.

Will drop that in later. Thanks!

On Wed, 22 Mar 2017 at 8:30 pm, Jan Blaha notifications@github.com wrote:

@pofider commented on this pull request.

In lib/scripts/conversionScriptPart.js https://github.com/pofider/phantom-html-to-pdf/pull/55#discussion_r107365064 :

@@ -76,6 +83,11 @@ page.onInitialized = function() { };

page.open(body.url, function () { +

  • page.evaluate(function() {
  • console.log(JSON.stringify(document.cookie));
  • });

I believe you could remove this code.

page.evaluate(function() { console.log(JSON.stringify(document.cookie)); });

Instead pass it in test to avoid unnecessary logging in real run. See the following, it could work?

it('should include user defined cookies in http requests', function(done) { conversion({ html: '',...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pofider/phantom-html-to-pdf/pull/55#pullrequestreview-28330293, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZlyXT6LoFmys0i1iWDyBwtXmGDWrhnks5roOo8gaJpZM4Mk7D1 .

chips5k commented 7 years ago

Done - let me know if all good now.

chips5k commented 7 years ago

Hi Jan - this is wierd, i had a lot of issues running the tests locally - randomly failing/working. Final tests ran successfully, but when i pushed - travis CI failed the dedicated process version of the test.

My local failures seemed to be related to read/write access to the temp dir. Can you let me know if i've done something wrong ?

pofider commented 7 years ago

Hmm, something is probably wrong in the test temp clean up. Not sure what it is at this moment. In every case this has nothing to do with this PR so I'm going to merge it.

pofider commented 7 years ago

Thank you, released in the latest 0.5.0