Replace PhantomJS with Puppeteer #46

kevva · 2017-09-24T18:03:46Z

Probably still needs some work:

Make sure we handle errors
Add script option
Add style option
Add some sort of hook that exposes page from puppeteer to users
Add keepAlive option with a .close method on the returned promise that lets users kill it manually (good when capturing many pages).
Code improvements?

kevva · 2017-09-24T18:05:05Z

screenshot.js

+const fileUrl = require('file-url');
+const isUrl = require('is-url-superb');
+
+module.exports = class Screenshot {


Does this class makes sense? I did it so that I wouldn't have to pass around page everywhere if I kept it functional. If we keep it, should we export it too maybe?

SamVerschueren

Haven't reviewed everything yet, don't have time now but already gave some remarks.

SamVerschueren · 2017-09-24T18:09:17Z

index.js

+	}
+
+	return puppeteer.launch()
+		.then(browser => browser.newPage()


Add an extra ) at the end

SamVerschueren · 2017-09-24T18:10:13Z

readme.md

-
-stream.pipe(fs.createWriteStream('google.com-1024x768.png'));
+screenshot('http://google.com', '1024x768', {crop: true}).then(data => {
+	fs.writeFileSync('google.com-1024x768.png');


fs.writeFileSync('google.com-1024x768.png', data);

SamVerschueren · 2017-09-24T18:12:14Z

readme.md

@@ -51,38 +51,19 @@ Default: `false`

 Crop to the set height.

-##### delay


Why was this removed? We could use delay to manually delay the capturing after opening the url, no?

This was added as a hack to wait for elements or other stuff to finish. We do this automatically now. Although there probably is some option for this in puppeteer.

It could be used by users as well to make sure an animation has ended. If you know a certain animation takes 2 seconds, someone could use delay: 2 to make sure he captures after the animation took place.

I never used it to be honest, but just finding a use case :).

I would use this. For example, on my website, I load the "latest blog post" widget async, so it's loaded a second after DOMContentLoaded.

I think we should switch to a wait/waitFor option instead that takes a selector, a number or a function and uses this method https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagewaitforselectororfunctionortimeout-options-args.

Yes, good idea, but I also like to keep the delay. Sometimes I just want a screenshot of a random website and ensure it’s fully loaded first.

SamVerschueren · 2017-09-24T18:15:26Z

readme.md

-
-Apply custom CSS to the webpage. Specify some CSS or the path to a CSS file.
-
-##### script


We could use page.addScriptTag(url).

Yup. In conjunction with injectFile(path).

SamVerschueren · 2017-09-24T18:15:49Z

readme.md


 ##### selector

 Type: `string`

 Capture a specific DOM element.

-##### css


Weird that we don't have a replacement for this... Should we open a new issue in Puppeteer?

Yes, open an issue on Puppeteer. In the meantime, we can use page.addScriptTag(url) to inject the CSS.

We can probably do this in page.evaluate() too.

Filed an issue puppeteer/puppeteer#892.

Just tested things out, we could do something like this with page.evaluate.

const headHandle = await page.$('head'); const result = await page.evaluate(head => { head.innerHTML += '<style>body { color: red; }</style>'; }, headHandle);

If css is a file path, we could just read the data with fs.readFile and pass in the contents with page.evaluate until, maybe in the future, we can replace that with a addStyleTag.

PR sent puppeteer/puppeteer#947

sindresorhus · 2017-09-24T18:22:27Z

@kevva What do you think about targeting Node.js 8? So we can use async/await. Would be so much nicer. It's going to be a major release anyway. I plan to do that in the next major of Pageres.

sindresorhus · 2017-09-24T18:25:00Z

screenshot.js

+	setHeaders(headers) {
+		if (typeof headers !== 'object') {
+			return Promise.resolve();
+		}


Doesn't make sense to make this optional (Same with the other methods). If a user calls setHeaders() they should be expected to provide some.

I don't disagree. I mainly did this for internal use to keep the code as clean as possible. If we target Node.js 8 I think we can get rid of the class altogether. I don't see why users would need these methods in this module.

sindresorhus · 2017-09-24T18:28:01Z

readme.md

-const stream = screenshot('http://google.com', '1024x768', {crop: true});
-
-stream.pipe(fs.createWriteStream('google.com-1024x768.png'));
+screenshot('http://google.com', '1024x768', {crop: true}).then(data => {


I wonder if we should just make the size argument part of the options object with a good default.

Yup, let's do that.

kevva · 2017-09-24T18:35:30Z

@kevva What do you think about targeting Node.js 8? So we can use async/await. Would be so much nicer. It's going to be a major release anyway. I plan to do that in the next major of Pageres.

👍 I agree. And it would be pretty future proof for a foreseeable future.

sindresorhus · 2017-09-24T18:40:54Z

I'm super excited we can finally get rid of PhantomJS! 🙌

kevva · 2017-09-24T21:43:50Z

Now uses async/await. Some tests are still failing though.

sindresorhus · 2017-09-25T05:58:08Z

index.js

+	const {
+		cookies, crop, format, headers, height, hide, keepAlive, password, scale,
+		script, selector, timeout, transparent, userAgent, username, width
+	} = opts;


This feels like overusing destructuring just for the sake of it. And using opts.cookies in the code makes it more explicit it's coming from a user option.

SamVerschueren · 2017-09-28T05:35:01Z

By the way, should we consider renaming this to screenshot instead of screenshot-stream, it's not a stream anymore :p.

kevva · 2017-10-03T20:16:54Z

By the way, should we consider renaming this to screenshot instead of screenshot-stream, it's not a stream anymore :p.

If we can get the name, yes. But it hasn't been updated in five years so shouldn't be impossible.

SamVerschueren · 2017-10-04T05:42:40Z

index.js

+	await page.goto(uri, opts);
+
+	if (script) {
+		const fn = isUrl(script) ? page.addScriptTag : script.endsWith('.js') ? page.injectFile : page.evaluate;


I wonder if we could write this line like this

const fn = isUrl(script) || script.endsWith('.js') ? page.addScriptTag : page.evaluate;

A script tag works for a local file as well right? If not, I think we should wrap the second block in parentheses for readability.

const fn = isUrl(script) ? page.addScriptTag : (script.endsWith('.js') ? page.injectFile : page.evaluate);

SamVerschueren · 2017-10-11T17:51:42Z

Added a PR to Puppeteer to make injecting js and css much easier. puppeteer/puppeteer#996

sindresorhus · 2018-03-05T20:43:14Z

What's left to do here?

kevva · 2018-03-09T14:42:12Z

Probably some small things to edge out. I'll take a look in the weekend.

c0bra · 2018-04-30T13:55:51Z

Any news on this? I'd rather not have to fork to get around some PhantomJS wonkiness.

cluk3 · 2018-10-28T11:18:46Z

I can help with this if needed!

sindresorhus · 2018-10-28T11:24:52Z

@cluk3 Help is most welcome. Open a PR based on this one. I'll review ;)

Replace PhantomJS with Puppeteer

12373a4

kevva commented Sep 24, 2017

View reviewed changes

Add missing devDependencies

6720fd7

SamVerschueren requested changes Sep 24, 2017

View reviewed changes

Add launch method to Screenshot class

08c161e

Typo

3cfae93

sindresorhus suggested changes Sep 24, 2017

View reviewed changes

Minor readme tweaks

53c5ee4

kevva added 2 commits September 24, 2017 23:28

Support Node.js >=8

0fc17a3

Make sure cookies is an Array

74864f4

kevva force-pushed the puppeteer branch from b5a187a to ccfe06e Compare September 24, 2017 22:02

Add keepAlive and browser options

f31a09b

kevva force-pushed the puppeteer branch 2 times, most recently from d350873 to f31a09b Compare September 24, 2017 22:44

sindresorhus reviewed Sep 25, 2017

View reviewed changes

SamVerschueren reviewed Oct 4, 2017

View reviewed changes

Use local fixtures

3bfddc5

Always use addScriptTag and addStyleTag

14a0354

SamVerschueren mentioned this pull request Oct 25, 2017

Possible to skip unloadable resource? #47

Closed

SamVerschueren mentioned this pull request Apr 10, 2018

No support for css grid? sindresorhus/pageres#328

Closed

		@@ -51,38 +51,19 @@ Default: `false`

		Crop to the set height.

		##### delay


		Apply custom CSS to the webpage. Specify some CSS or the path to a CSS file.

		##### script

Replace PhantomJS with Puppeteer #46

Are you sure you want to change the base?

Replace PhantomJS with Puppeteer #46

Conversation

kevva commented Sep 24, 2017 • edited Loading

kevva Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

SamVerschueren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevva Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamVerschueren Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

kevva Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevva commented Sep 24, 2017

sindresorhus commented Sep 24, 2017

kevva commented Sep 24, 2017

Choose a reason for hiding this comment

SamVerschueren commented Sep 28, 2017

kevva commented Oct 3, 2017 • edited Loading

SamVerschueren Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

SamVerschueren commented Oct 11, 2017

sindresorhus commented Mar 5, 2018

kevva commented Mar 9, 2018

c0bra commented Apr 30, 2018

cluk3 commented Oct 28, 2018

sindresorhus commented Oct 28, 2018

kevva commented Sep 24, 2017 •

edited

Loading

kevva Sep 24, 2017 •

edited

Loading

kevva Sep 24, 2017 •

edited

Loading

sindresorhus Sep 25, 2017 •

edited

Loading

SamVerschueren Sep 27, 2017 •

edited

Loading

sindresorhus commented Sep 24, 2017 •

edited

Loading

kevva Sep 24, 2017 •

edited

Loading

kevva commented Oct 3, 2017 •

edited

Loading

SamVerschueren Oct 4, 2017 •

edited

Loading