puppeteerLoad should reject Errors

[zepumph]

over in https://github.com/phetsims/perennial/issues/273 @mattpen and I found that most of the usages of puppeteer were doing extra work to tease out the error from the result of puppeteerLoad. It is much cleaning to just reject those errors and handle them accordingly.

[zepumph]

We can use the patch in https://github.com/phetsims/perennial/issues/273#issuecomment-1341145143 to start.

[zepumph]

All seemed to go well enough here to commit. I wanted to tag @jonathanolson so he is aware in case he wants to discuss this direction further.

I tested with studio-fuzz and it worked well.

I wanted to use this issue to remove all usages of page.goto, but I couldn't quite get them all. There are some that use features of puppeteer that I do not recommend adding to puppeteerLoad like exposeFunction

[jonathanolson]

Makes sense to me.

[zepumph]

Here are the usages of page.goto I wasn't able to convert and that I do not recommend trying to:

• puppeteerQUnit (expose many functions)
• getFromSimInMaster (exposeFunction)
• generatePhetioMacroAPI (exit based on receiving a console message)
• getPreloads (decaf is old code that doesn't need active maintenance)

[zepumph]

@jonathanolson will you please give puppeteerLoad and a couple of usages (like in ReleaseBranch) a spot check?

[jonathanolson]

If I'm reading this correctly, we'll reject the promise on error, the await promise on line 93 will throw an error, and the page/browser close won't be run. This seems like it leaks those things if there is an error encountered. Presumably we'll need to fix that? Also committed some doc improvements above.

[jonathanolson]

Also, if the page is closed, evaluations will return null. Presumably we'd want this to error instead? If I'm executing a puppeteerEvaluate on a page that I might get a null result, I wouldn't want something to silently return null if the page is closed.

[jonathanolson]

If our page doesn't load within the timeout, we'll reject (great!). However we still seem to execute the load() code (since we never set loaded = false on the timeout). This could run page.evaluate AFTER page.close (or with some undefined order).

[jonathanolson]

I'll be testing the ReleaseBranch changes (it will require some time). It looks fine, but was a bit tricky to understand at first (I documented the unanticipated-to-me fact that withServer returns the return value of its callback). I fully realize I was the one who added the code to make it do that...