Screenshots. #24

rhyek · 2017-08-01T23:22:58Z

Related: #13.
This is a work in progress. I need to know if this is the approach you were thinking of.

Basically:

await hpc.create(html, options) // pdf
await hpc.create(html, options, 'pdf') // pdf
await hpc.create(html, options, 'screenshot') // screenshot
await hpc.createPDF(html, options) // pdf
await hpc.createScreenshot(html, options) // screenshot

CreateOptions now has:

  screenshotOptions?: {
    /**
     * The options to pass to Chrome's Page.captureScreenshot.
     *
     * @type {ChromeCaptureScreenshotOptions}
     */
    captureScreenShotOptions?: CaptureScreenshotOptions;

    /**
     * The options to pass to Chrome's Emulation.setDeviceMetricsOverride.
     *
     * @type {DeviceMetricsOverrideOptions}
     */
    deviceMetrics?: DeviceMetricsOverrideOptions;

    /**
     * Wether to capture a full-page screenshot.
     *
     * @type {boolean}
     */
    fullPage?: boolean;
  };

codecov · 2017-08-01T23:25:55Z

Codecov Report

Merging #24 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #24   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines         108    157   +49     
  Branches       12     17    +5     
=====================================
+ Hits          108    157   +49

Impacted Files	Coverage Δ
src/index.ts	`100% <100%> (ø)`	⬆️
src/Generators.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c695d1...af7054a. Read the comment docs.

rhyek · 2017-08-04T00:08:25Z

Not sure what is causing those test errors. I'll have a look at it later. @westy92 have you had a chance to take a look at this?

tomasc · 2017-08-06T08:55:07Z

Thank you @rhyek, very nice!
I think this is a good approach, I like how the interface matches the PDF one. I assume all the CompletionTrigger options would be the same, correct?

rhyek · 2017-08-06T09:16:43Z

They should work the same, yes. Only thing missing should be tests.

rhyek · 2017-08-07T02:59:14Z

Hm, well I was sort of expecting some tests to fail due to timeouts where OCR scanning is involved. The other tests I noticed are basically inconsistencies in OCR scanning vs my environment, which I wasn't expecting. Things like HELLO! being interpreted as HELLO'. Those are easy to fix.
Others are sort of just random timeouts that I've noticed happen occasionally.

Thoughts on these tests?

rhyek · 2017-08-07T03:12:54Z

To be honest, the OCR thing was mostly me having a little fun with writing these tests. I'm honestly more inclined to just remove them altogether instead of figuring these inconsistencies out. IMO, with the PDF tests, screenshots could be assumed to work fine for the most part and a couple of tests should suffice.

tomasc · 2017-08-07T07:56:08Z

Thank you, @rhyek. Perhaps an idea might be to split this PR into two – one for adding the screenshot interface, other for additional OCR option. That way the screenshot feature could be merged and new package released, and the OCR added separately later on. What do you think?

rhyek · 2017-08-07T08:05:36Z

Yeah, not a bad idea. I just wish @westy92 would share his opinion. Atm I have no idea when or if this might be merged and published.

tomasc · 2017-08-07T08:14:53Z

@rhyek @westy92 usually reacts immediately – I guess he might be on vacation or so.

westy92 · 2017-08-07T12:47:15Z

@rhyek Thank you for the contribution! I plan to take a first look tonight. Sorry for the delay!

rhyek · 2017-08-25T18:46:02Z

It has been nearly a month since I made this PR. Are there any issues that I can help resolve so this can get merged?

…ausing tests to fail

rhyek · 2017-09-16T14:23:48Z

I cleaned up the tests mostly by removing the OCR ones which I feel were unneeded. The failed tests are are an issue of slow processing on the test server.

If there isn't any interest in this PR please close.

tomasc · 2017-09-18T10:20:29Z

@westy92 why is this not being merged please? This would be a welcome addition to the library as far as I am concerned.

graingert · 2017-09-19T14:22:50Z

@rhyek why did you close this?

rhyek · 2017-09-19T14:42:38Z

@graingert, sorry. I interpreted the lack of attention to this PR as the repo owner having either no interest or different plans for this feature, which is fine.

graingert · 2017-09-19T15:08:56Z

@westy92 do you know what's happening with this PR?

westy92 · 2017-09-19T18:26:45Z

I still have not had the time to review this in depth. Since this is a new (large) feature, I plan to investigate methods of generating images, verify this is the best one, and then think about how I want this new functionality integrated into the project. It will take a lot of time that I have not had yet. I just reopened this, as I will get to it eventually.

westy92 · 2017-09-19T18:28:41Z

If anyone else wishes to review this changeset, please do. The more eyes, the better.

graingert · 2017-09-19T18:29:14Z

Do you think it might be worth splitting the image generation from the chrome process reuse via tabs?

…

On 19 Sep 2017 19:26, "Seth Westphal" ***@***.***> wrote: Reopened #24 <#24>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTAis8TZmvLFW5unFolrFmXQvKKZfks5skAdmgaJpZM4OqaqJ> .

westy92 · 2017-09-19T18:34:59Z

@graingert I don't see any code here working with tabs, but that implementation should remain separate.

rhyek · 2017-09-19T18:51:42Z

@westy92, he means this.

I am already using my fork in production and since multiple PDFs and/or screenshots are being generated concurrently, I have found my application crashes frequently without the tab functionality.

For example, I have a page that displays several (20 or so) screenshots at the same time all generated in real-time with a single chrome instance. Without tabs it does not work.

It is very basic since it is just utilizing chrome-remote-interface's API. IMO, the code for both screenshots and tabs is very simple. I think it would be a good idea to review both at the same time in this PR.

There is already a test that utilizes tabs. Currently it only generates 5 concurrent PDFs since the global 15s timeout and slow processing of the test server doesn't allow for more, for some reason. That test itself could use a review as well.

graingert · 2017-09-22T14:32:59Z

.gitignore

+
+# tesseract.js eng language files
+test/langs/*
+!test/langs/.gitkeep


missing trailing new line

graingert · 2017-09-22T14:38:28Z

@rhyek do you have a fork on npm? and can you separate out the use of tabs for concurrency?

rhyek · 2017-09-22T17:25:32Z

I'm using my fork from Github, directly. Changing this branch would not affect me at all. Nevertheless, are your plans for tabs much different than what I have implemented? Like I said, this is already working for me in production and is transparent to the user. The implementation can be changed at any point since it is not part of the public API.

…

On Fri, Sep 22, 2017 at 8:38 AM, Thomas Grainger ***@***.***> wrote: @rhyek <https://github.com/rhyek> do you have a fork on npm? and can you separate out the use of tabs for concurrency? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq5yrYsJ6JelYS84i1DejYjIxAv_wBAks5sk8ZkgaJpZM4OqaqJ> .

graingert · 2017-09-22T18:26:00Z

@rhyek can you separate out the fix to use tabs for concurrency into a different PR?

rhyek · 2017-09-22T19:17:58Z

Sure. I think I'll have some time over the weekend.

graingert · 2017-09-25T18:41:27Z

@rhyek I see you didn't have time over the weekend, but don't worry!

I've extracted from your PR the bits required for parallel generation of PDFs here: #61 would be good to get a review/rebase from you!

westy92 · 2017-11-22T05:55:02Z

@rhyek I think the project is at a decent position to get this merged in! Could you please sync this with master? Thanks!

rhyek · 2017-11-22T14:00:11Z

Sorry, I have 0 interest in this PR. If someone else wants to help you out with a new one with my code that's fine.

westy92 · 2017-11-22T15:01:59Z

Thank you for your contributions so far! I'll take it from here and use this as a starting point.

straldev · 2018-04-02T19:44:22Z

@westy92 What's the status on this PR?

gregbown · 2020-08-01T19:30:37Z

@westy92 Was this ever completed? I have the need to render a screenshot prior to agreement because displaying a final pdf in the browser would be equivalent to free. This would be very convenient and streamline the document process.
@rhyek I read that you use this feature in production but had issues with crashing related to tabs. Did you resolve this issue?

Is this project active? I hope Puppeteer has not eaten everybody's lunch?

vivex · 2020-12-01T06:29:45Z

any update on this?

westy92 · 2022-04-19T01:15:13Z

Screenshot functionality has been published in version 0.8.0! 🎉

rhyek added 2 commits August 1, 2017 17:15

WIP

57c84ff

createPdf -> createPDF

fed18e8

rhyek changed the title ~~[WIP] Screenshots. Closes #13.~~ [WIP] Screenshots. Aug 1, 2017

--hide-scrollbars

994af05

rhyek added 4 commits August 1, 2017 17:41

remove+ignore package-lock.json; remove sleep in screenshot generation

adf9845

DocumentType -> OutputType

4269240

using tabs for concurrent usage

cac291f

unused Runtime

9b9e85c

rhyek mentioned this pull request Aug 6, 2017

Allow creating multiple PDFs in parallel #14

Closed

rhyek added 2 commits August 6, 2017 19:45

added tests for screeshot generator

4d326b7

added concurrent pdf generation test

a3fefa2

Repository owner deleted a comment from codecov bot Aug 9, 2017

westy92 self-assigned this Aug 10, 2017

rhyek added 2 commits September 12, 2017 17:07

removed screenshot testing with ocr due to its unreliability and it c…

7afc997

…ausing tests to fail

fix conflicts

af7054a

rhyek changed the title ~~[WIP] Screenshots.~~ Screenshots. Sep 16, 2017

rhyek closed this Sep 18, 2017

westy92 reopened this Sep 19, 2017

westy92 added the enhancement label Sep 19, 2017

graingert reviewed Sep 22, 2017

View reviewed changes

.gitignore

# tesseract.js eng language files

test/langs/*

!test/langs/.gitkeep

Copy link

graingert Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing trailing new line

graingert mentioned this pull request Sep 25, 2017

use multiple tabs #61

Closed

westy92 closed this Apr 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Screenshots. #24

Screenshots. #24

rhyek commented Aug 1, 2017 •

edited

Loading

codecov bot commented Aug 1, 2017 •

edited

Loading

rhyek commented Aug 4, 2017 •

edited

Loading

tomasc commented Aug 6, 2017

rhyek commented Aug 6, 2017

rhyek commented Aug 7, 2017

rhyek commented Aug 7, 2017

tomasc commented Aug 7, 2017

rhyek commented Aug 7, 2017 •

edited

Loading

tomasc commented Aug 7, 2017

westy92 commented Aug 7, 2017

rhyek commented Aug 25, 2017

rhyek commented Sep 16, 2017

tomasc commented Sep 18, 2017

graingert commented Sep 19, 2017

rhyek commented Sep 19, 2017

graingert commented Sep 19, 2017

westy92 commented Sep 19, 2017

westy92 commented Sep 19, 2017

graingert commented Sep 19, 2017 via email

westy92 commented Sep 19, 2017

rhyek commented Sep 19, 2017

graingert Sep 22, 2017

graingert commented Sep 22, 2017

rhyek commented Sep 22, 2017 via email •

edited

Loading

graingert commented Sep 22, 2017

rhyek commented Sep 22, 2017

graingert commented Sep 25, 2017

westy92 commented Nov 22, 2017

rhyek commented Nov 22, 2017

westy92 commented Nov 22, 2017

straldev commented Apr 2, 2018

gregbown commented Aug 1, 2020

vivex commented Dec 1, 2020

westy92 commented Apr 19, 2022

Screenshots. #24

Screenshots. #24

Conversation

rhyek commented Aug 1, 2017 • edited Loading

codecov bot commented Aug 1, 2017 • edited Loading

Codecov Report

rhyek commented Aug 4, 2017 • edited Loading

tomasc commented Aug 6, 2017

rhyek commented Aug 6, 2017

rhyek commented Aug 7, 2017

rhyek commented Aug 7, 2017

tomasc commented Aug 7, 2017

rhyek commented Aug 7, 2017 • edited Loading

tomasc commented Aug 7, 2017

westy92 commented Aug 7, 2017

rhyek commented Aug 25, 2017

rhyek commented Sep 16, 2017

tomasc commented Sep 18, 2017

graingert commented Sep 19, 2017

rhyek commented Sep 19, 2017

graingert commented Sep 19, 2017

westy92 commented Sep 19, 2017

westy92 commented Sep 19, 2017

graingert commented Sep 19, 2017 via email

westy92 commented Sep 19, 2017

rhyek commented Sep 19, 2017

graingert Sep 22, 2017

Choose a reason for hiding this comment

graingert commented Sep 22, 2017

rhyek commented Sep 22, 2017 via email • edited Loading

graingert commented Sep 22, 2017

rhyek commented Sep 22, 2017

graingert commented Sep 25, 2017

westy92 commented Nov 22, 2017

rhyek commented Nov 22, 2017

westy92 commented Nov 22, 2017

straldev commented Apr 2, 2018

gregbown commented Aug 1, 2020

vivex commented Dec 1, 2020

westy92 commented Apr 19, 2022

rhyek commented Aug 1, 2017 •

edited

Loading

codecov bot commented Aug 1, 2017 •

edited

Loading

rhyek commented Aug 4, 2017 •

edited

Loading

rhyek commented Aug 7, 2017 •

edited

Loading

rhyek commented Sep 22, 2017 via email •

edited

Loading