Add evaluate() function #181
Conversation
darron1217
changed the title
|
I've also added tests error fix |
tests/BrowsershotTest.php
Outdated
| @@ -162,20 +162,6 @@ public function it_can_handle_a_permissions_error() | |||
| ->save($targetPath); | |||
| } | |||
|
|
|||
| /** @test */ | |||
| public function it_can_use_the_methods_of_the_image_package() | |||
There was a problem hiding this comment.
I removed this test because method named format() is overrided. So we cannot access to spatie/image package's function.
(I guess format() function is implemented for setting pdf format)
tests/BrowsershotTest.php
Outdated
| @@ -201,6 +187,7 @@ public function it_can_create_a_command_to_generate_a_screenshot() | |||
| 'height' => 1080, | |||
| ], | |||
| 'args' => [], | |||
| 'type' => 'png', | |||
There was a problem hiding this comment.
Why is 'png' added in every test? Can this be removed?
There was a problem hiding this comment.
It related to #180
This change is not related with this PR. Should I remove it?
(I just tried to remove Travis Error)
bin/browser.js
Outdated
| output = await page[request.action](request.options); | ||
| if(request.action == 'evaluate') { | ||
| output = await page.evaluate(request.options.pageFunction); | ||
| } else { |
There was a problem hiding this comment.
Can you refactor this, so we don't need an else statement?
First thought: let's introduce a getOutput function. You can use an early return in that function.
There was a problem hiding this comment.
Can you explain more about getOutput?
You mean request.action == 'getOutput'?
There was a problem hiding this comment.
I removed explicit base64 encoding code.
But I can't make it more simpler because passing request.options can't handle evaluate() function which needs string argument
There was a problem hiding this comment.
That else statement can be avoided by introducing a new function getOutput(request) that returns the output.
There was a problem hiding this comment.
I implemented it on d33189d
Is it the function you said?
darron1217
changed the title
|
I've implemented getOutput() on browser.js |
|
The tests seem to be failing. Can you also add a test for the |
|
The test error is caused by another accepted PR #180 |
|
Thanks! Could you rebase to the current master? |
|
Okay wait for a second |
|
I rebased it, and commits are dirty... haha Wait a second, it's weird |
|
I don't understand how to rebase it.... |
|
#185 |
I've created a PR to add puppeteer's
evaluate()functionhttps://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pageevaluatepagefunction-args