OnetapInc / chromy

Chromy is a library for operating headless chrome. 🍺🍺🍺
MIT License
605 stars 41 forks source link

Is Sharp still a dependency? #59

Closed garris closed 6 years ago

garris commented 6 years ago

You mentioned here that the Sharp dependency will be going away. I had a user attempt to install yesterday that this was still happening. Can you let me know the status?

https://github.com/garris/BackstopJS/issues/479#issuecomment-319873591

kensoh commented 6 years ago

I browsed through Chromy code some time back and I saw that the Sharp module is used for resizing / clipping images I think. Even with latest Canary or Chrome having ability to do that without Sharp, I think Shinji still needs to make modifications to enable doing through newer Chrome versions.

But I guess that would mean incompatibility for users running older Chrome versions.

garris commented 6 years ago

Chrome is evergreen so older versions should not be a big consideration. Yes?

dotneet commented 6 years ago

@kensoh Thanks! Exactly!

@garris The ability has been merged in canary 5 weeks ago. So it might ship in chrome61 I think. Current stable version of Chrome is ver.60 so I can't remove the dependency yet.

garris commented 6 years ago

I see. Ok thanks everyone for the update!

Tomorrow -- I am going to try to update all the Backstop documentation and merge in the chromy branch to master!

kensoh commented 6 years ago

Wow, that is fantastic! That's fast the migration :smile:

garris commented 6 years ago

Thanks for all your support and kind words -- I am very very excited about this development!

dotneet commented 6 years ago

@garris That's great!

By the way, I want to ask you about #60. If a string in scenario.selectors contains single quote, does this line work correctly? https://github.com/garris/BackstopJS/blob/0ff6d3e462904512d586bdd09495e2b0438636a3/core/util/runChromy.js#L352

I didn't find selector problem in chromy.

garris commented 6 years ago

Oh -- apologies -- I see what's going on there... Maybe I should add a method to force escape any quotes in the string? What do you think?

dotneet commented 6 years ago

I think so.

In chromy, all selectors are passed through this function.

function escapeSingleQuote (string) {
  if (typeof string !== 'string') {
    return string
  }
  return string.replace(/'/g, '\\\'')
}
garris commented 6 years ago

great. will do that! Thanks!

kensoh commented 6 years ago

Another nice (and rapid) collaboration win :smile:

garris commented 6 years ago

@dotneet The sharp issue is causing confusion for windows users. Would it be possible for you to go ahead and implement the sharp removal fix on a feature branch?

I would use the feature branch in BackstopJS until you merge that to master.

I think this is important because it is much easier for end users to install chrome canary than it is for them to debug npm installs. Also, it would enable you to have your fix ready when chrome finally promotes their canary version.

Maybe there is a way to detect what version chrome is installed on a users machine -- and throw an error if minimum required version is missing?

Please let me know what you think. Thank you!

dotneet commented 6 years ago

@garris I've published v0.5.0 @beta. Please try it. I've removed the dependency but I had a problem. As I thought chrome v61 has an ability to achieve to remove the dependency but It was misunderstand. In some cases, the ability(clipping feature) that is contained in Chrome61 doesn't works well. So I've replaced sharp with Jimp to remove a dependency to native module. Jimp is a pure javascript library.

dotneet commented 6 years ago

Here is example of getting chrome version.

   chromy.evaluate(_ => {
      let v = navigator.userAgent.match(/Chrom(e|ium)\/([0-9]+)\./)
      return v ? parseInt(v[2], 10) : false
    })
garris commented 6 years ago

This is great news -- thank you for both responses! I will update BackstopJS tonight. Cheers.

kensoh commented 6 years ago

Jimp is a nice library thanks for sharing :) I have the image-clipping issue as well when I integrate Chrome with CasperJS. So I capture the webpage screen shot from Chrome, load it in the running CasperJS / PhantomJS process and then use CasperJS / PhantomJS method to clip the image. I use the workaround in order to avoid Node.js dependency, but I dunno how long I can do that.

dotneet commented 6 years ago

@kensoh Using a browser to clip the image might be the one way to remove dependency. The problem I encountered is caused by scaling. clip parameter in Page.captureScreenshot() has scale parameter. When we use scale parameter Chrome automatically change the viewport size. This behavior changes the scroll-position and displayed area. If we take a full screenshot, maybe It's not a problem.

I'm not sure this is possible. I want to try this when I have a time.

  1. capture screenshot as original scale.
  2. show the image in new tab
  3. capture the new tab as full screenshot with the scale we desired.
kensoh commented 6 years ago

I see.. Thanks for your sharing @dotneet :) Yes I wasn't ready to move into scaling and emulation for mobile-devices, so I didn't implement those features for my project, which is more for automating manual processes than testing. So these advanced use cases are not relevant.

There are so many things to take care of in browser automation projects, including emulating touch, dealing with headers, cookies etc. Amazed at all the work you put into the project, very challenging!

garris commented 6 years ago

@dotneet. I am suspecting creating a canvas element instead of browser window is potentially faster (i would be very interested to measure this in ms). You could instantiate the canvas inside the existing window, then, import the screenshot, crop, and export the cropped image.

dotneet commented 6 years ago

@garris that's a great idea! I'll consider both ideas and use the idea that have high performance and less side effects.

garris commented 6 years ago

The new version is beautiful. Thanks!