percy / percy-cli-ruby

[Deprecated] Command-line interface for Percy.
https://percy.io/docs/clients/ruby/cli
MIT License
19 stars 11 forks source link

Percy doesn't wait for web components to resolve. #1

Closed dbatiste closed 8 years ago

dbatiste commented 8 years ago

I've been using Percy for a little while now to produce perceptual diffs of static test pages. I am looking at implementing as Polymer web component, but am having difficulty obtaining the correct snapshot. I suspect this is a timing issue related to the web component not yet being resolved at the time of the snapshot (I can point to example if needed). Currently my test page resides here - it is simply a test page for a color palette. Is this expected to work?

dbatiste commented 8 years ago

After some poking around I discovered more snapshot options, specifically --enable_javascript in lib/percy/cli.rb. I'd thought this had solved the issue, but apparently not. I think perhaps Polymer is not being loaded.

dlockhart commented 8 years ago

I think the underlying issue is that web components aren't immediately resolved when the page loads. This is true with Polymer but also in general with web components. The WebComponentsReady event exists for this purpose.

What would be ideal is if we pass a CLI argument that would tell Percy to wait for the WebComponentsReady event. That would require no code changes on our end.

Alternatively, a CLI argument to tell Percy to wait for a custom event (e.g. percy-ready) to fire. Then in our snapshot file we could do something like this:

window.addEventListener('WebComponentsReady', function() {
    console.log('WCs Ready');
    window.dispatchEvent(new CustomEvent('percy-ready'));
});

This first is obviously less work inside the actual snapshot files, and would avoid having snapshot supporting code inside production files. We can help contribute this if you like... with a bit of direction! :)

fotinakis commented 8 years ago

I'm so sorry, I didn't actually see this issue until now because for some reason I wasn't watching this repository, so I didn't get an email about it! :( Big apologies, I try to be quickly responsive about these things!

I'm sure that the problem is this: we disabled JavaScript in our rendering pipeline on Mar 13, because I was thinking about it and discovered a potential security vulnerability that was only possible by allowing arbitrary JavaScript execution. Because --enable-javascript was unadvertised and I only saw a couple snapshots that had ever used it, I decided to just disable JavaScript for the time being until someone needed it and I had time to preventatively fix the security problem. Clearly, you need this now! :) I have a few possible fixes in mind, it's somewhat convoluted networking work so it's not an instant fix, but I'll prioritize it and let you know when we've deployed new rendering infrastructure. I'll try to have this done by next week and will let you know, sorry for the delay.

fotinakis commented 8 years ago

Ok, this should be fixed and you should be able to --enable_javascript again. Let me know if everything works ok!

dlockhart commented 8 years ago

@fotinakis thanks Mike. We're noticing that JavaScript is now running again, however it still hasn't quite solved the underlying issue -- just uncovered another issue. :)

After a bunch of digging involving writing out the error console to the page so it shows up in the diff (ha!), I think I've narrowed it down to a CORS-related problem. The way HTML5 imports work is via the <link> tag: <link rel="import" href="myImport.html">

CORS applies to these imports, and it appears as though you're proxying this stuff through http://proxyme.percy.io -- which doesn't have CORS enabled. I think if you just allowed CORS from that proxy, our problem would be solved. Not sure what underlying web server you're using there, but typically they all have a way to enable CORS.

fotinakis commented 8 years ago

Interesting!

In this case I don't think CORS is the problem because the domain is the same—we internally serve all the uploaded assets from the same fake domain, proxyme.percy.io, so when proxyme.percy.io/button.html tries to relative load button.html.ignore it will be on the same domain and not require CORS.

I dug through our logs a bit and it looks like you're not actually uploading the component file: [PROXY_NOTICE][44906] Serving 404 for: http://proxyme.percy.io/button.html.ignore

Which is probably caused by the fact that we only upload certain files and it doesn't include .ignore files: https://github.com/percy/percy-cli/blob/v1.2.2/lib/percy/cli/snapshot.rb#L13

If .ignore is a standard polymer thing (is it?) I'm happy to add it to our STATIC_RESOURCE_EXTENSIONS. Unfortunately it looks like I didn't include a mechanism for customizing allowed resource extensions (ie. I built a --snapshot_regex flag, but not a --resource_regex flag, it's hard-coded right now). We should definitely provide a simple CLI flag for people to customize the allowed extensions for what we call "build resources" that are uploaded alongside the root HTML files. I think a flag like --include_extensions=js,ignore,csv would be pretty easy to add.

Maybe you can fork this gem first and let me know if manually adding .ignore to the allowed extensions makes everything work as expected?

dbatiste commented 8 years ago

Thanks for looking at this Mike. The .ignore is not a Polymer thing. That's just us naming the file and excluding it from GitHub since it's a generated test resource. We can easily change that filename if need-be.

dlockhart commented 8 years ago

Yeah ideally it has a ".html" extension, but if we do that then Percy includes it as one of the static files to snapshot. Just to test it out though, I tried a run with it just as an HTML file (vulcanized.html): https://percy.io/Brightspace/valence-ui-button/builds/67270

Still not working, and still throwing some kind of error when it tries to load that file...

fotinakis commented 8 years ago

So, more weirdness. I downloaded both of those files and ran them locally. In Chrome, button.html looks fine. In Firefox (which we use in Percy's rendering pipeline) however, the page loads verrry slowly—the buttons stay in their original state for a long time, and then switch to their styled versions.

The Firefox console shows this:

mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create

Here's a video of this happening: https://youtu.be/52pXTk9cVVc

However, when I went to do it again, the behavior seems to be gone. :( If it's actually happening though it could explain why the page is being captured in the early, broken state. Do you know if there is a way to fix that console warning above, or is it a polymer-internal required thing?

Also, it sort of makes sense that vulcanized.html doesn't load directly and is blank but the other one does. If I try to load vulcanized.html locally in Firefox it gives this in console:

- mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create vulcanized.html:266:0
- TypeError: document.registerElement is not a function vulcanized.html:321:0
- TypeError: Polymer.Base._getExtendedPrototype is not a function
dlockhart commented 8 years ago

Thanks for the continuing support on this Mike. Seems like it's related to this issue with the web components polyfill. That issue is closed, but talks a lot about performance being really slow (hanging) in Firefox, but mostly only bad when the developer tools console is open. I'm seeing the same -- very slow loading when the console is open (with the same warning you're seeing), but when I close the console, things render quickly. Maybe just not quickly enough though?