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 to do with page.evaluateAsync? #11

Closed
aslushnikov opened this issue Jun 14, 2017 · 3 comments
Closed

What to do with page.evaluateAsync? #11

aslushnikov opened this issue Jun 14, 2017 · 3 comments

Comments

@aslushnikov
Copy link
Contributor

aslushnikov commented Jun 14, 2017

Today, there are two methods in page's API:

  • page.evaluate which evaluates code in the page and returns result.
  • page.evaluateAync which too evaluates code in the page and, if the evaluation result is promise, waits for promise to resolve and returns the promise value.

The page.evaluate is straightforward, but page.evaluateAsync has cryptic name and hard to grasp. Should we remove the page.evaluateAsync and always await promise in page.evaluate?

If we want to keep them separate, is there a better name for the page.evaluateAsync?


Illustrating with examples, the following snippet returns "42":

var Browser = require('puppeteer').Browser();
var browser = new Browser();
browser.newPage().then(async page => {
    console.log(await page.evaluate(() => 6 * 7)); // prints '42'
    browser.close();
});

However, if we try to return a promise, we will get an empty object as a result:

var Browser = require('puppeteer').Browser();
var browser = new Browser();
browser.newPage().then(async page => {
    console.log(await page.evaluate(() => Promise.resolve(6 * 7))); // prints '{}'
    browser.close();
});

In order to get the '42' as a returned value, one would need to use page.evaluateAsync:

var Browser = require('puppeteer').Browser();
var browser = new Browser();
browser.newPage().then(async page => {
    console.log(await page.evaluateAsync(() => Promise.resolve(6 * 7))); // prints '42'
    browser.close();
});

This kind of situation could be avoided if we were to await promise in page.evaluate.


For the record: in case of merging evaluate with the evaluateAsync methods, we should make sure that evaluating the code from-inside the inpagecallback works successfully (which is a bit tricky since page is on a pause)

@aslushnikov
Copy link
Contributor Author

Guys, I would appreciate your feedback on this! cc @ebidel @paulirish @dgozman

@ebidel
Copy link
Contributor

ebidel commented Jun 14, 2017

FWIW, from our experience w/ Lighthouse, lots of the code we've needed to eval in the page needs to be wrapped in a promise. Basically, anything non-trivial the user will be doing. Also, most of the web platform APIs have moved towards promises, so devs are used to dealing with them. async/await only make things easier.

I like the idea of one method. Simplify + keep things async. Though, do we want to match the Phantom API as much as possible? It might be easier to transition existing users.

@aslushnikov
Copy link
Contributor Author

Phantom's evaluateAsync is a totally different beast, (which is not needed in our case since most of puppeteer's api's return promises). This is a good argument towards removing puppeteer's evaluateAsync - to avoid confusion.

Though, do we want to match the Phantom API as much as possible?

I think we want to strike a good balance between having a familiar API while not missing the opportunity to improve. So if there are a few options to implement X and there is no clear winner, we should pick the one which is more familiar for phantom users.

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

No branches or pull requests

2 participants