Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What is the reason of using a sync version of writeFile for pdf save? #884

Closed
tsabolov opened this issue Sep 26, 2017 · 5 comments
Closed
Labels

Comments

@tsabolov
Copy link

https://github.com/GoogleChrome/puppeteer/blob/9b0a06216e5b364aecaef6931d7844a91bebe46e/lib/Page.js#L588
Why wasn't async version of writeFile used for writing the pdf?

@paulirish
Copy link
Collaborator

I presume there isn't a strong reason.. :)

If you wanted to change it, I'm sure the eng team would appreciate a pull request.

@Garbee
Copy link
Contributor

Garbee commented Sep 26, 2017

If you do use the async version, it is still going to be awaited for processing. So is there any functional difference between the two at that juncture from the point of view of code operation?

@paulirish
Copy link
Collaborator

@Garbee reading lib/fs and this post it seems like the big difference is that the node event loop is blocked with sync, so any events coming from the chrome child_process (for example) would not be recognized until after.

And there's this issue suggesting that sync will always have inferior performance characteristics.

i can't think of a reason we NEED sync here, so might as well do the right thing.

@Garbee
Copy link
Contributor

Garbee commented Sep 26, 2017

big difference is that the node event loop is blocked with sync

SGTM then.

@paulirish
Copy link
Collaborator

Ah. there's already a PR for this: #879

@aslushnikov aslushnikov added the P1 label Oct 4, 2017
ithinkihaveacat pushed a commit to ithinkihaveacat/puppeteer that referenced this issue Oct 31, 2017
This patch:
- introduces `helper.promisify` - a simple polyfill for the `util.promisify`. The 
  `util.promisify` could not be used due to Node6 compatibility issues.
- migrates all sync filesystem operations to the async replicas

Fixes puppeteer#884.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants