-
Notifications
You must be signed in to change notification settings - Fork 571
pdf() API #84
pdf() API #84
Conversation
246e703
to
99565b3
Compare
In EDIT: There's now an open issue for this at #113. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to see the const screenshot
fix, but otherwise looks good to me!
docs/api.md
Outdated
__Example__ | ||
|
||
```js | ||
const screenshot = await chromeless |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean const pdf
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Sloppy copy/pasting on my part.
|
||
// write to `/tmp` instead | ||
else { | ||
const filePath = `/tmp/${cuid()}.pdf` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look into using a module that helps with tmp folders. This file tmp
doesn't exist in windows systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With node 4 and above os.tmpdir() should do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be addressed as part of #113.
Bucket: process.env['CHROMELESS_S3_BUCKET_NAME'], | ||
Key: s3Path, | ||
ContentType: 'application/pdf', | ||
ACL: 'public-read', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably this should be configurable as well with an env variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be addressed as part of #113.
Key: s3Path, | ||
ContentType: 'application/pdf', | ||
ACL: 'public-read', | ||
Body: new Buffer(data, 'base64'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also use Buffer.from()
as new Buffer()
is deprecated API in newer Node.js versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be addressed as part of #113.
5acc4ea
to
f764f4e
Compare
@joelgriffith I fixed the documentation error in f764f4e. |
As for @lirantal's comments, I figure it's better to keep the implementation of |
src/chrome/local-runtime.ts
Outdated
|
||
// check if S3 configured | ||
if (process.env['CHROMELESS_S3_BUCKET_NAME'] && process.env['CHROMELESS_S3_BUCKET_URL']) { | ||
const s3Path = `${cuid()}.pdf` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we save both screenshots and pdfs to the same bucked and to the same folder.
What do you think of putting them like bucket/screenshots/${cuid()}.png
and bucket/pdfs/${cuid()}.pdf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be left alone in this pull request, and this can be addressed as part of #113.
src/util.ts
Outdated
export async function pdf(client: Client): Promise<string> { | ||
const {Page} = client | ||
|
||
const pdf = await Page.printToPDF() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be hard to allow user pass options here? In particular landscape
may be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tried to do that before I opened this PR, and I couldn't get it to work. Even when I hard-coded landscape: true
here, it would save a portrait PDF.
Try changing line 310 to the following:
const pdf = await Page.printToPDF({landscape: true})
It does not appear to change the output. I tried messing around with options such as scale
and paperHeight
, and that did not appear to affect anything either.
I'm not quite sure why passing in an options object has no effect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options do work at the CDP/Chrome level, so there may be an issue somewhere else. Perhaps the options aren't being passed along correctly—also the initial support for printing to pdf in headless-chrome was somewhat limited—are you trying in a recent version of Chrome (or via the Proxy?)
Page.printToPDF({
landscape: false,
displayHeaderFooter: false,
printBackground: true,
scale: 1,
paperWidth: 8.27, // aka A4
paperHeight: 11.69, // aka A4
marginTop: 0,
marginBottom: 0,
marginLeft: 0,
marginRight: 0,
pageRanges: '',
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Passing these options did not work when I was using Chrome 59, but they work now that I've upgraded to Chrome 60. I'm on macOS Sierra.
Now that I know that this works, I'll implement the ability for the user to pass in these options.
I'm really interested in the PDF feature. Is there something I could help with or would there be too many devs poking at the same feature? |
Hi @kimmobrunfeldt It looks like @seangransee has nearly finished the feature—let's give him a bit more time to wrap it up. |
I'll try to get to this today. @kimmobrunfeldt, if you urgently need it in the meantime, the feature is totally usable in this branch. The only thing missing is the ability to pass in options, so right now the PDF will be generated using the defaults. |
f764f4e
to
2cee98f
Compare
@vladgolubev @adieuadieu The API can now optionally accept an It's implemented in 67e72ef. Feel free to take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, we should follow-up with the changes needed in #113
In case anyone's interested in seeing some sample PDFs this generates:
|
🔥 once we get another maintainer approval we'll merge this (hopefully get it staged for 1.1.0 release) |
src/types.ts
Outdated
@@ -82,6 +82,10 @@ export type Command = | |||
type: 'returnHtml' | |||
} | |||
| { | |||
type: 'returnPDF', | |||
options: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some type definitions here instead of object
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schickling Take a look at 7db33a1. I'm not very familiar with TypeScript, so feel free to correct any mistakes I may have made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! :)
docs/api.md
Outdated
@@ -392,9 +392,24 @@ console.log(screenshot) // prints local file path or S3 URL | |||
|
|||
<a name="api-pdf" /> | |||
|
|||
### pdf() - Not implemented yet | |||
### pdf(options?: object) - Chromeless<string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if object
was a concretly documented type
2cee98f
to
7db33a1
Compare
Using examples from the top and running on the master, I am getting |
|
@gorangajic is correct. I'm getting that error as well when using Maybe the switch to |
@gorangajic, take a look at #146. The error goes away for me when I manually start chrome in headless mode before running the example. |
Page.printToPDF()
documentation:https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-printToPDF
You can test this the same way as the screenshot example: