for-GET / http-decision-diagram

An activity diagram to describe the resolution of HTTP response status codes, given various headers.
https://github.com/for-GET/http-decision-diagram/blob/master/doc/2013-06-10-http-hell-no.md
Apache License 2.0
3.61k stars 196 forks source link

Upgrade JointJS to v1.0 #46

Closed christopheradams closed 4 years ago

christopheradams commented 7 years ago

I found it worthwhile to try upgrading JointJS to version 1.0 since it seems to drastically decrease the diagram loading time.

I retained the toSVG and toPNG plugins from joint.js 0.8 since they still work, but are not included in 1.0. No changes to the coffeescript file or the diagram were necessary.

The jointjs and canvg libraries are installed with NPM and Bower. Do you prefer them to be vendored (committed to the repo)?

This PR also makes two unrelated changes:

andreineculau commented 7 years ago

Hi! This is great!

A very fast feedback is no bower/npm so that we can keep publishing via github-pages (jekyll).

re: "Encode URI instead of base64" is that supported in "all" browsers now? I thought it was problematic

christopheradams commented 7 years ago

OK, I can vendor the deps. I'll push an update.

The base64 line didn't seem to work after upgrading jointjs, hence the change. I didn't dig into the reason, so it needs another look.

christopheradams commented 7 years ago

Ok, the dependencies are now committed.

I'm still not sure what's going on with the base64 line. It just doesn't work for me with the SVG generated by JointJS 1.0. The PNG never opens up, but there's no error in the console. That line works just fine on master.

christopheradams commented 7 years ago

I updated it to be the same as this line:

img.src = 'data:image/svg+xml;base64,' + btoa(unescape(encodeURIComponent(svg)));

I don't understand how these characters are appearing in the SVG source when doing the encoding:

accept￿charset�matches
300�MULTIPLE�CHOICES

It happens to all of the FSM labels. This creates an invalid SVG file.

Other workarounds are:

img.src = 'data:image/svg+xml; charset=utf8, '+encodeURIComponent(svg);

Or using this function:

function b64EncodeUnicode(str) {
    return btoa(encodeURIComponent(str).replace(/%([0-9A-F]{2})/g, function(match, p1) {
        return String.fromCharCode('0x' + p1);
    }));
}
christopheradams commented 7 years ago

The problem character is a non-break space. It's due to this line in JointJS: https://github.com/clientIO/joint/blob/master/dist/joint.js#L2166

return (text || '').replace(/ /g, '\u00A0');

This means the SVG has to be encoded/escaped as above, or by replacing that character:

svg = svg.replace(/\u00A0/g, " ");
andreineculau commented 7 years ago

interesting. based on the comments jointjs left above that line it seems like a test gimmick rather than a real fix/good practice (I don't understand the role that IE plays in this). the commit history seems to say the same "now everything is testable" https://github.com/clientIO/joint/blame/c36e5bd43ebb68d3584f7471fae44caf706f910e/src/vectorizer.js#L954

so is it by any chance a bug that needs to be filed to jointjs? regardless of whether we have the non-breaking-space-to-breaking-space replacement or not, I'd like to know the background of it, so that we know if we should watch for jointjs stop doing the sanitizeText.

On Mon, Feb 13, 2017 at 6:01 AM, Christopher Adams <notifications@github.com

wrote:

The problem character is a non-break space. It's due to this line in JointJS: https://github.com/clientIO/joint/blob/master/dist/joint.js#L2166

return (text || '').replace(/ /g, '\u00A0');

This means the SVG has to be encoded/escaped as above, or by replacing that character:

svg = svg.replace(/\u00A0/g, " ");

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/for-GET/http-decision-diagram/pull/46#issuecomment-279295108, or mute the thread https://github.com/notifications/unsubscribe-auth/AArOQTJCfDhKmROmiJumDNzELkAUWYhHks5rb-O0gaJpZM4L76hO .

-- andreineculau.com http://www.andreineculau.com

andreineculau commented 7 years ago

thanks once again for the time you're putting into this

On Mon, Feb 13, 2017 at 8:48 AM, Andrei Neculau andrei.neculau@gmail.com wrote:

interesting. based on the comments jointjs left above that line it seems like a test gimmick rather than a real fix/good practice (I don't understand the role that IE plays in this). the commit history seems to say the same "now everything is testable" https://github.com/clientIO/joint/blame/c36e5bd43ebb68d3584f7471fae44c af706f910e/src/vectorizer.js#L954

so is it by any chance a bug that needs to be filed to jointjs? regardless of whether we have the non-breaking-space-to-breaking-space replacement or not, I'd like to know the background of it, so that we know if we should watch for jointjs stop doing the sanitizeText.

On Mon, Feb 13, 2017 at 6:01 AM, Christopher Adams < notifications@github.com> wrote:

The problem character is a non-break space. It's due to this line in JointJS: https://github.com/clientIO/joint/blob/master/dist/joint.js# L2166

return (text || '').replace(/ /g, '\u00A0');

This means the SVG has to be encoded/escaped as above, or by replacing that character:

svg = svg.replace(/\u00A0/g, " ");

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/for-GET/http-decision-diagram/pull/46#issuecomment-279295108, or mute the thread https://github.com/notifications/unsubscribe-auth/AArOQTJCfDhKmROmiJumDNzELkAUWYhHks5rb-O0gaJpZM4L76hO .

-- andreineculau.com http://www.andreineculau.com

-- andreineculau.com http://www.andreineculau.com

christopheradams commented 7 years ago

so is it by any chance a bug that needs to be filed to jointjs?

I don't know if it's a bug or not, but the long comment would lead me to conclude it's intentional behavior.

The rasterization plugins for JointJS are no longer open source, and the old version of the code in this repo predates the addition of this no-break space workaround for IE.

Perhaps it just surfaced a problem of calling btoa() directly on a Unicode string from the DOM.

This might be relevant: https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding