search

domenic / svg2png

Converts SVGs to PNGs, using PhantomJS
Other
585 stars 134 forks source link

Add support for custom PhantomJS implementation #87

Closed ryan-codingintrigue closed 7 years ago

ryan-codingintrigue commented 7 years ago

Added an option parameter for a custom PhantomJS "instance".

This is heavily based off the phantomjs-node API, which supports most PhantomJS calls as well as being promise-based. Although, I'm sure it could be modified to work with other PhantomJS wrapper libraries.

This allows the caller to be in control of spinning up the PhantomJS instance - meaning that we can share a single instance across multiple calls to svg2png.

ryan-codingintrigue commented 7 years ago

@domenic I had some issues getting all of the existing integration tests to pass. I'm sure it's some syntax/error checking around the options logic but I can't see it. Would be grateful if you could review & let me know where I've gone wrong?

domenic commented 7 years ago

Don't worry too much about the tests; they seem to be system-dependent for some reason. You can try to sanity-check the output yourself though of npm run rebaseline (but don't commit those).

I'm not excited about this PR as-is because it duplicates the code in converter.js into a new file phantomNode.js. Please find a way to not do that. Maybe phantom-node is not a good fit for this project if you can't run normal phantomjs scripts inside of it.

ryan-codingintrigue commented 7 years ago

Thanks, I got there eventually using rebaseline.

Yeah, the code needed to be duplicated since there is a slight API difference in page.evaluate (sync vs. Promise) and I couldn't reuse converter.js since it uses stdin/out.

Oh well, thanks for taking a look 👍