Connect to Puppeteer Endpoint, Update Puppeteer to 2.0.0 #19
Conversation
* update dependency * manually compare images visually for regressions * update hashes
| - `options` <[Object]> Pupetteer [connect options](https://github.com/puppeteer/puppeteer/blob/master/docs/api.md#puppeteerconnectoptions) object which will typically contain a `browserWSEndpoint` property. | ||
| - returns: <[SvgToImg]> an instance which is connected with the specified Puppeteer endpoint. | ||
|
|
||
| Use this method if you want to connect to a running Puppeteer endpoint (see [here](https://github.com/etienne-martin/svg-to-img/issues/9)). Note that the actual connection is established lazily; this means this method will always return, even when the given `browserWSEndpoint` cannot be reached -- in this case, errors will be thrown later when calling one of the `to` functions. |
There was a problem hiding this comment.
I think this portion of the doc is not up-to-date with the actual behavior.
errors will be thrown later when calling one of the
tofunctions
Looking at the tests, it seams the connect method will throw if it can't connect to the websocket endpoint.
There was a problem hiding this comment.
Actually, the test case was not quite accurate (connect should be outside of try). I updated the test code to reflect this. The readme is actually correct.
dist/helpers.js
Outdated
| @@ -93,6 +93,6 @@ exports.renderSvg = async (svg, options) => { | |||
| img.addEventListener("load", onLoad); | |||
| img.addEventListener("error", onError); | |||
| document.body.appendChild(img); | |||
| img.src = "data:image/svg+xml;charset=utf8," + svg; | |||
| img.src = "data:image/svg+xml;base64," + btoa(svg); | |||
There was a problem hiding this comment.
I don't understand why the svg string needs to be base64 encoded because of the websocket endpoint. Should it just work the same regardless if you're connected to a local Chromium instance or a remote browser?
There was a problem hiding this comment.
When running via WebSocket I get the "Malformed SVG" error triggered in onError.
Admittedly, I have no real clue why this only happens when running remotely (my naive assumption would probably be that the WebSocket connection breaks something with the transmitted SVG string/function?). 🤔
I've added a code comment why the encoding is necessary.
* Add `contributors` section to `package.json` * Add global ignore rule for `max-classes-per-file` * Add explicit `IConnectOptions` interface instead of using the Puppeteer default * Tests: Move line of of `try` block to make clear when exception is thrown * Add some more explanation to `readme.md` * Remove temporary testing code * Add comment why base64 encoding is necessary
|
@etienne-martin We're currently still investigating a potential issue |
Fixes issue with `btoa` and “The string to be encoded contains characters outside of the Latin1 range.” when encoding non-ASCII characters
|
@etienne-martin Okay, mentioned issue is fixed with previous commit ( |
|
@etienne-martin Any more feedback so far? :-) |
|
@etienne-martin Are you still interested in integrating this? Else wise I’d push a forked version to NPM. |
|
Yes sorry I've been super busy lately! |
|
No worries -- just wanted to double check. I’d really like to avoid having a fork on NPM, so I’ll be patient instead :-) Cheers! |
qqilihq
force-pushed
the
connect-puppeteer
branch
from
691e755 to
ce367c1
Compare
|
FYI: In case anyone needs this, we made this fork available on |
This PR adds the possibility to connect to a Browserless endpoint using
browserWSEndpointas described in #9renderSvginhelpers.tsaccordinglyindex.tsto allow to keep the necessary statereadme.mdOpen questions:
I personally would prefer to use the
puppeteer-coredependency in this library (andpuppeteeronly withindevDependecies). This way, Chromium would not automatically be pulled. People who want to use an “embedded” Chromium could still achieve this by explicitly addingpuppeteerto their dependencies. However this would be a breaking change.With the current version, for people who do not want the “embedded” Chromium, there’s the
PUPPETEER_SKIP_CHROMIUM_DOWNLOADflag (see updated readme.md) which would need to be set before running installation via npm resp. yarn.Kudos also go to @danielesser who helped testing this!