Convert problem in Safari

[nawroth]

I have received reports of this error and similar when using Safari:

Error
Exception:
asciidoctor: FAILED: : Failed to parse source, undefined is not an object (evaluating 'a.$chr')

I have set up older versions of DocGist, but that doesn't seem to help. Loading minimal documents usually works, but I haven't been able to track down the actual problem. The front page of DocGist fails every time (it's a rather complex page).

As I don't have any OSX hardware, it's really hard for me to dig any deeper into this.

NOTE: If window.console doesn't exist when the page is loaded, it's replaced by a fake one that silently does nothing.

@mojavelinux Does the above error message point you in any direction?

[mojavelinux]

I just resurrected my MacBook for my device testing lab, so I can look into it and try to figure out what's happening. My suspicion is that it's in Asciidoctor.js. undefined is not an object is the Opal version of a null pointer exception.

[mojavelinux]

This appears to be a max line length issue in Safari (perhaps brought on by the use of jQuery, perhaps not, hard to get a read on it). The minimized Asciidoctor.js file is created by uglify with a max line length setting of 32,000 characters. If you switch to using an non-minified version of Asciidoctor.js, it immediately starts working in Safari. It also works if you activate the Development Tools (aka Web Inspector) (as strange as that seems).

I think the fix here is to switch to a more conservative line length when minimizing Asciidoctor.js. I've read reports that firewalls will sometimes truncate lines that exceed 500 characters, so that seems to be the generally accepted conservative number. (See http://webmasters.stackexchange.com/questions/47776/what-is-the-maximum-safe-line-length-in-css-files).

Here's how we'd configure that in the Grunt build for Asciidoctor.js:

glify: {
  dist: {
    options: {
      maxLineLen: 500 
    },  
    files: {
      'dist/npm/asciidoctor-core.min.js': ['build/npm/asciidoctor-core-min.js'],
      ...
    }   
  }
},

That seems to increase the asciidoctor-all.min.js file from 595K to 596K and it makes debugging a hell of a lot easier, so it's totally worth it.

[mojavelinux]

Note that I do the same thing with the default stylesheet. I always include endlines so that it can be viewed in an editor, even if all the other characters are stripped. See https://github.com/asciidoctor/asciidoctor/blob/master/data/stylesheets/asciidoctor-default.css. It just makes life easier.

[mojavelinux]

Can you file the issue upstream in Asciidoctor.js?

On a side note, I recommend using npm to manage the version of Asciidoctor.js in the repository so that it is easier to tell which version is in use. Right now, it's really hard to track.

Also, did you know that you can get configure GitHub to use master as the GitHub Pages branch so that you don't have to push to two different branches?

[nawroth]

@mojavelinux Great that you found the issue. I filed a PR.

For the version of Asciidoctor.js, you can do git log -- js/asciidoctor-all.min.js or visit https://github.com/asciidoctor/docgist/commits/master/js/asciidoctor-all.min.js (which looks slightly weird at the moment, as I pushed what's in my PR to asciidoctor.js)

But yeah, all dependencies should be managed. I just want to keep the project layout, it makes for a great workflow. We tried a grunt-based setup for GraphGist but reverted it to keep things simple! BTW GraphGist is now a ruby-backed application, so it doesn't use Asciidoctor.js any more.

I've been a bit trigger happy for a while, but I do like the possibility to push to master without getting it deployed to gh-pages as well. If we want to simplify this, I'd rather keep only the gh-pages branch to make it clear what happens when you push to it or merge a PR. I could just keep commits in my own repo until Ithink they are ready to deploy.

[mojavelinux]

you can do git log -- js/asciidoctor-all.min.js

Sure, but it's still a manual association, so you have to trust the git comment (or do a diff to verify).

But yeah, all dependencies should be managed.

What I'm hoping for is that the dependency manager (npm, bower, etc) just updates the file being checked in. That way, if you run the build locally, it should verify that the files are the same or show a modification if they aren't in sync. So still a manual update, but at least there is metadata in the project and not just the git log. (more transparency I think)

We tried a grunt-based setup ...

I'd take a note from @Mogztter and use an npm script. That eliminates the need for a build tool and it can still work without running it (using it just to update).

[nawroth]

It seems like we still have issues with Safari, you can try it out from http://gist.asciidoctor.org/ Maybe we can at least get a useful stacktrace now. @mojavelinux

[ggrossetie]

Could you post the stacktrace on GitHub/Gist ? I don't have a Mac and recent versions of Safari are only available on Mac OS :(

Thanks Le 28 déc. 2015 11:40 AM, "Anders Nawroth" notifications@github.com a écrit :

It seems like we still have issues with Safari, you can try it out from http://gist.asciidoctor.org/ Maybe we can at least get a useful stacktrace now. @mojavelinux https://github.com/mojavelinux

— Reply to this email directly or view it on GitHub https://github.com/asciidoctor/docgist/issues/27#issuecomment-167540536.

[mojavelinux]

I'll boot up the lab equipment and see where we stand.

[mojavelinux]

Even though I'm going to give it a try, it would be extremely helpful if we knew exactly what the report is you are getting, @nawroth. For instance, as I mentioned in my last tests, if I had the Developer Tools open, DocGist worked perfectly every time. If I closed the Developer Tools, it hung. I could never get a stacktrace because it would never fail when I could see the Console. (The error you reported did show when I tried the Asciidoctor.js example).

[nawroth]

@mojavelinux Sorry, try here: http://nawroth.github.io/docgist/

[mojavelinux]

I've gotten to the bottom of this. The problem is uglify. It creates invalid JavaScript (which most browsers seem to be able to stomach, but not Safari). This is why would should absolutely not be using uglify. Instead, we should be using the Closure JavaScript compiler. The Closure compiler is created and used by Google, so I trust that they've tested all the browsers thoroughly.

I ran the unminified asciidoctor-all.js through the closure compiler using:

$ closure-compiler --charset UTF-8 --js asciidoctor-all.js --js_output_file asciidoctor-all.min.js

I then moved the result over the DocGist and tried it. It worked right away in Safari.

There are some more advanced options would could try to enable, but the default settings (simple optimizations) already produces a file which is smaller than what uglify creates...and it's correct. (It also defaults to a max line width of 500).

The Closure compiler is available as an RPM, DEB or installable via Brew on OSX).

[mojavelinux]

Btw, the error that we are seeing in Safari isn't a real error. It just means "it doesn't work somewhere, so here's a completely random failure". :)

[mojavelinux]

@nawroth I noticed that you aren't actually parsing the header only the first time. That's because you are passing "parse_header_only" as an attribute instead of an option. It should be adjacent to the "to_file" option and have a real boolean value.

[mojavelinux]

In other words, it needs to be set here: https://github.com/asciidoctor/docgist/blob/master/js/docgist.js#L101

[nawroth]

I created a minified version along the instructions, and as Chrome and Firefox didn't complain, I pushed the change to http://gist.asciidoctor.org/

@mojavelinux I fixed the parse_header_only issue, thanks!

[mojavelinux]

Confirmed it worked without a glitch on Safari!

[mojavelinux]

I do notice a sequence of flashes in all browsers when rendering that page. I think it would be better to perform the postprocessing before you append the HTML to the DOM, or continue hiding the DOM node until the postprocessing is complete.

[nawroth]

@mojavelinux Most of the flashes were due to how Mathjax was loaded. Changing that improved things significantly. It's still a bit random though, and a single flash often happens. But the insane "going back and forth three times" rendering should be gone.

[mojavelinux]

Nice! I'll check it out.

[mojavelinux]

Much better.

The only noticeable flash that occurs now is that the font is changing in the navigation bar at the top. Perhaps load that CSS for that earlier so that the font family doesn't change after it is shown?

[nawroth]

It's the other way around: it's loaded early, but then overridden by the Asciidoctor CSS later on. I'll add style rules so it's not so easily affected by the Asciidoctor theme.

I did a run on Saucelabs and the front page rendered just fine on different iPads, iPhones and various OSX versions. Safari 9 on El Capitan got stuck once, but worked fine on the next instance.

[mojavelinux]

Speaking of which, DocGist is yet another tool that could greatly benefit from the embeddable stylesheet. https://github.com/asciidoctor/asciidoctor-stylesheet-factory/issues/18

We really need to find time to make that happen because it's going to make life so much simpler. The standalone stylesheet simply has too many styles that get in the way for use cases like this one.

[ggrossetie]

This is why would should absolutely not be using uglify. Instead, we should be using the Closure JavaScript compiler. The Closure compiler is created and used by Google, so I trust that they've tested all the browsers thoroughly.

I will give it a try.

"Closure-compiler requires java to be installed and in the path." :disappointed: The closure compiler service doesn't allow to send "big" JavaScript files: https://developers.google.com/closure/compiler/docs/api-ref

Maybe we can make the task optional, otherwise anyone who want to contribute to Asciidoctor.js will have to install : Ruby, Node and Java...

[mojavelinux]

Certainly, the minify task should be optional to build and test the project.

The minification step is only something that needs to be enabled for the release profile. From my tests both recent and past, the closure compiler is the only reliable way to minify JavaScript. (I had always intended to use it, hence why it was in the Gemfile from the start, see https://github.com/asciidoctor/asciidoctor.js/blob/master/Gemfile#L16-L20).

[mojavelinux]

Of course, we could also setup our own service to run the closure compiler if we really need to. We can put that DigitalOcean server to use.