Galooshi / happo

Visual diffing in CI for user interfaces
504 stars 16 forks source link

Curly quotes in description name breaks #164

Closed ljharb closed 7 years ago

ljharb commented 7 years ago

I used curly quotes in a description name, and the diffs failed to generate.

I don't have any error output available.

trotzig commented 7 years ago

Thanks for the report. I'm unable to reproduce this however. I have this example:

happo.define('{foo}', function() {
  var elem = document.createElement('div');
  elem.innerHTML = 'Text inside';
  document.body.appendChild(elem);
});

And it doesn't cause any failure when running happo run:

⨠ happo run
== Sinatra (v1.4.7) has taken the stage on 4567 for development with backup from Thin
Thin web server (v1.7.0 codename Dunder Mifflin)
Maximum connections set to 1024
Listening on 0.0.0.0:4567, CTRL+C to stop
desktop (1024x768)
├─  {foo} ..... No diff.
└─  bar ..... No diff.
== Sinatra has ended his set (crowd applauds)
ljharb commented 7 years ago

@trotzig i don't see any “ ” ‘ ’ in your example name? Curly quotes, not curly braces :-)

trotzig commented 7 years ago

Gah, I didn't read properly... Let me have a second look.

trotzig commented 7 years ago

That isn't causing any problems for me either.

happo.define('“foo” ‘foo’', function() {
  // ...
});
ljharb commented 7 years ago

fwiw, our current code does:

happo.define(`<${name}> ${description}`, …

and "description" had something like foo: “bar”.

trotzig commented 7 years ago

Yeah, that seems to work for me as well.

I suspect this is a charset issue. We send the javascript file down through a Sinatra send_file call. We're not specifying charset here, so some default somewhere is probably making sure that utf-8 is used/assumed.

We're currently in the process of replacing Sinatra (and Ruby) with Express and Node. If you could find a workaround until that project is done, that would be great. I'm not too eager about digging up a fix for this in Ruby land. I hope that feels okay with you?

ljharb commented 7 years ago

Sure, my workaround for now is to use straight quotes. I'd prefer to leave this issue open until there's a release that fixes it, but it seems fine to wait until a node rewrite :-)

trotzig commented 7 years ago

Yes, let's leave it open. I'll try to remember to check back in here when we have something to test.

lencioni commented 7 years ago

To be clear, the failure happens on the uploaded diffs page. Here's the error in the console:

Uncaught DOMException: Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range.(…)r 
@ index.html:210o 
@ index.html:210_constructComponentWithoutOwner 
@ react.min.js:13_constructComponent 
...

More info on this can be found here: http://stackoverflow.com/questions/30106476/using-javascripts-atob-to-decode-base64-doesnt-properly-decode-utf-8-strings

trotzig commented 7 years ago

Ah, okay. So I'm guessing this is related to the linking of each example ("bookmarking" feature). It should be easy to fix with a different escaping method.

trotzig commented 7 years ago

I pushed a fix for this on the convert-to-js branch. We changed some stuff around so I decided not to base this off of the current master/release. The fix should go out in the first 3.0 release.

lencioni commented 7 years ago

I think this is fixed in 3.0.0.