Fix ampersand in jsx href and src #1056

Merged
merged 18 commits into from Apr 8, 2017

Conversation

Projects
None yet
5 participants
@yamafaktory
Contributor

yamafaktory commented Mar 20, 2017

This should fix #1051.

yamafaktory added some commits Mar 20, 2017

@@ -578,7 +578,7 @@ exports[`parens.js 2`] = `
<path
key='1'
d='M13.6,10.6l,4-2.8L9.5,M13.6,10.6l,4-2.8L9.5,M13.6,10.6l,4-2.8L9.5,M13.6,10.6l,4-2.8L9.5,M13.6,10.6l,4-2.8L9.5,M13.6,10.6l,4-2.8L9.5,'
- />,

This comment has been minimized.

@yamafaktory

yamafaktory Mar 20, 2017

Contributor

😲

@yamafaktory

yamafaktory Mar 20, 2017

Contributor

😲

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Mar 20, 2017

Contributor

Hmm, why does prettier escape HTML at all? Shouldn't it leave the code as is except for styling?

Contributor

Pajn commented Mar 20, 2017

Hmm, why does prettier escape HTML at all? Shouldn't it leave the code as is except for styling?

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Mar 20, 2017

Contributor

Hmm, I read in old issues that the raw text is apparently gone in the parser, which is sad :(

There are a lot of attributes that takes data and not a string to display in HTML, not just those that take urls. And even for only those that takes urls it's impossible to cover all cases due to user defined attributes...

Maybe at least add xlinkHref as well which is common with react together with SVG?

Contributor

Pajn commented Mar 20, 2017

Hmm, I read in old issues that the raw text is apparently gone in the parser, which is sad :(

There are a lot of attributes that takes data and not a string to display in HTML, not just those that take urls. And even for only those that takes urls it's impossible to cover all cases due to user defined attributes...

Maybe at least add xlinkHref as well which is common with react together with SVG?

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 20, 2017

Contributor

Thanks for the feedback! It's not up to me to decide on that topic, I basically was trying to fix the aforementioned issue. Let's involve @vjeux & @jlongster in that discussion 👍.

Contributor

yamafaktory commented Mar 20, 2017

Thanks for the feedback! It's not up to me to decide on that topic, I basically was trying to fix the aforementioned issue. Let's involve @vjeux & @jlongster in that discussion 👍.

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Mar 20, 2017

Contributor

I did not mean to lay it on you (or anyone else). Sorry if that's how it read.
What I meant is that the general state is sad as it can cause bugs in many cases and depending on where it make take a while to notice (or find, as your probably didn't expect that side effect when formatting your code).
However this is good, because even if the underlaying problem can be fixed, a quick fix for the common cases is a nice start 👍

Contributor

Pajn commented Mar 20, 2017

I did not mean to lay it on you (or anyone else). Sorry if that's how it read.
What I meant is that the general state is sad as it can cause bugs in many cases and depending on where it make take a while to notice (or find, as your probably didn't expect that side effect when formatting your code).
However this is good, because even if the underlaying problem can be fixed, a quick fix for the common cases is a nice start 👍

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 20, 2017

Contributor

No worries at all 👍, great feedback is what pushes OSS forward! Let's just see what the core team thinks about it ❤️.

Contributor

yamafaktory commented Mar 20, 2017

No worries at all 👍, great feedback is what pushes OSS forward! Let's just see what the core team thinks about it ❤️.

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Mar 20, 2017

Contributor

Ugh, never mind me. JSX apparently decodes the HTML entities itself, interesting.

Contributor

Pajn commented Mar 20, 2017

Ugh, never mind me. JSX apparently decodes the HTML entities itself, interesting.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 21, 2017

Collaborator

This is not the right fix. We need to escape the strings, not disable escaping altogether for some keys.

Right now we always transform & into &amp; but we don't need to most of the times. We only need to when it's ambiguous.

So, whenever the parser wouldn't convert it to an entity, we can avoid writing &amp;

Here's the code that is responsible for it:

Collaborator

vjeux commented Mar 21, 2017

This is not the right fix. We need to escape the strings, not disable escaping altogether for some keys.

Right now we always transform & into &amp; but we don't need to most of the times. We only need to when it's ambiguous.

So, whenever the parser wouldn't convert it to an entity, we can avoid writing &amp;

Here's the code that is responsible for it:

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 21, 2017

Contributor

Ok, I think I have a fix, will push that later.

Contributor

yamafaktory commented Mar 21, 2017

Ok, I think I have a fix, will push that later.

yamafaktory added some commits Mar 21, 2017

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 21, 2017

Contributor

@vjeux I refactored it based on the heuristic you defined before.

Contributor

yamafaktory commented Mar 21, 2017

@vjeux I refactored it based on the heuristic you defined before.

src/printer.js
- res = '"' + util.htmlEscapeInsideDoubleQuote(n.value.value) + '"';
+ // Whenever the parser wouldn't convert it to an entity
+ // do not escape.
+ if (isParserKeepingEntities(n, options)) {

This comment has been minimized.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

As you told me, I use this helper function in order to check if the original value is different or not from what the parser returns.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

As you told me, I use this helper function in order to check if the original value is different or not from what the parser returns.

src/printer.js
@@ -3297,6 +3303,19 @@ function printArrayItems(path, options, printPath, print) {
return concat(printedElements);
}
+function isParserKeepingEntities(node, options) {

This comment has been minimized.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

Here comes the new helper function, depending on the node type it performs a simple check with adjustments. If the parser wouldn't convert it to an entity, we should not do it too.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

Here comes the new helper function, depending on the node type it performs a simple check with adjustments. If the parser wouldn't convert it to an entity, we should not do it too.

This comment has been minimized.

@vjeux

vjeux Mar 22, 2017

Collaborator

I'm okay with this approach, but here's how I would like it. Have a function getJSXRawValue(node) that checks if node.extra.rawValue or node.raw exists and return it, otherwise read from the original source.

@vjeux

vjeux Mar 22, 2017

Collaborator

I'm okay with this approach, but here's how I would like it. Have a function getJSXRawValue(node) that checks if node.extra.rawValue or node.raw exists and return it, otherwise read from the original source.

src/printer.js
@@ -654,7 +654,7 @@ function genericPrintNoParens(path, options, print) {
var isTypeAnnotation = n.type === "ObjectTypeAnnotation";
var isTypeScriptTypeAnnotaion = n.type === "TSTypeLiteral";
// Leave this here because we *might* want to make this
- // configurable later -- flow accepts ";" for type separators,

This comment has been minimized.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

Trailing space removal from my Emacs configuration, sorry if it bloats the diff.

@yamafaktory

yamafaktory Mar 21, 2017

Contributor

Trailing space removal from my Emacs configuration, sorry if it bloats the diff.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 22, 2017

Contributor

@vjeux can you please review it again?

Contributor

yamafaktory commented Mar 22, 2017

@vjeux can you please review it again?

yamafaktory added some commits Mar 22, 2017

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 22, 2017

Contributor

@vjeux I was playing around with your solution. This could work but to rely on the parser raw value involves a different output, e.g. single quotes can be maintained and I don't know if this is what we want...

Contributor

yamafaktory commented Mar 22, 2017

@vjeux I was playing around with your solution. This could work but to rely on the parser raw value involves a different output, e.g. single quotes can be maintained and I don't know if this is what we want...

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 22, 2017

Contributor

One example is:

<div id='"&#39;<>&amp;quot;' />;

Before it was turned into:

<div id="&quot;'<>&amp;quot;" />;

And that's why a few tests are failing.

Contributor

yamafaktory commented Mar 22, 2017

One example is:

<div id='"&#39;<>&amp;quot;' />;

Before it was turned into:

<div id="&quot;'<>&amp;quot;" />;

And that's why a few tests are failing.

yamafaktory added some commits Mar 25, 2017

Merge branch 'master' of https://github.com/jlongster/prettier into 1…
…051-fix-ampersand-in-jsx-href-and-src
@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 25, 2017

Contributor

@vjeux your opinion regarding the latest comments.

Contributor

yamafaktory commented Mar 25, 2017

@vjeux your opinion regarding the latest comments.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 29, 2017

Contributor

@vjeux your thoughts regarding the previous comments? My first implementation was basically working, the new one needs refinements but I'm not quite sure about it.

Contributor

yamafaktory commented Mar 29, 2017

@vjeux your thoughts regarding the previous comments? My first implementation was basically working, the new one needs refinements but I'm not quite sure about it.

@rattrayalex

This comment has been minimized.

Show comment
Hide comment
@rattrayalex

rattrayalex Apr 6, 2017

Collaborator

As of babel/babylon#344 , which shipped in 7.0.0-beta8, we should just be able to use the extra.raw value.

EDIT: and it looks like that's the version that's on master. @yamafaktory would you like to update this PR to use that, or shall I submit a new PR?

Collaborator

rattrayalex commented Apr 6, 2017

As of babel/babylon#344 , which shipped in 7.0.0-beta8, we should just be able to use the extra.raw value.

EDIT: and it looks like that's the version that's on master. @yamafaktory would you like to update this PR to use that, or shall I submit a new PR?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 6, 2017

Collaborator

Okay, so I chatted with @rattrayalex and here's the plan of action we came up with:

We want to return the unprocessed raw value so that encoding is left as the user inputted it. We can now do that since the babylon issue was fixed (Thanks @rattrayalex for pushing it through!) and in master (Thanks @hzoo and @existentialism !).

There is just one small thing. If we are converting a string from a single quote to double quote, we need to make sure that double quotes " are converted to &quot;. It's as simple as a regex search, we don't need to care about escaping.

Collaborator

vjeux commented Apr 6, 2017

Okay, so I chatted with @rattrayalex and here's the plan of action we came up with:

We want to return the unprocessed raw value so that encoding is left as the user inputted it. We can now do that since the babylon issue was fixed (Thanks @rattrayalex for pushing it through!) and in master (Thanks @hzoo and @existentialism !).

There is just one small thing. If we are converting a string from a single quote to double quote, we need to make sure that double quotes " are converted to &quot;. It's as simple as a regex search, we don't need to care about escaping.

yamafaktory added some commits Apr 7, 2017

Merge branch 'master' of https://github.com/jlongster/prettier into 1…
…051-fix-ampersand-in-jsx-href-and-src
Update getJsXRawValue in order to return the unprocessed raw value.
Latest Babylon version includes a fix that allow us to directly
inject the unprocessed raw value available in the `extra.rawValue`
property of the node. A last transformation is applied by replacing
double quotes to `&quot;` entities.
@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Apr 7, 2017

Contributor

@vjeux / @rattrayalex I've updated the implementation to match your inputs, thanks!

Contributor

yamafaktory commented Apr 7, 2017

@vjeux / @rattrayalex I've updated the implementation to match your inputs, thanks!

@@ -1200,7 +1200,9 @@ function genericPrintNoParens(path, options, print) {
(n.value.type === "StringLiteral" || n.value.type === "Literal") &&
typeof n.value.value === "string"
) {
- res = '"' + util.htmlEscapeInsideDoubleQuote(n.value.value) + '"';

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

Can you delete that function from util since it's no longer being used?

@vjeux

vjeux Apr 7, 2017

Collaborator

Can you delete that function from util since it's no longer being used?

src/printer.js
@@ -1200,7 +1200,9 @@ function genericPrintNoParens(path, options, print) {
(n.value.type === "StringLiteral" || n.value.type === "Literal") &&
typeof n.value.value === "string"
) {
- res = '"' + util.htmlEscapeInsideDoubleQuote(n.value.value) + '"';
+ // Whenever the parser wouldn't convert it to an entity

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

Let's remove this comment, it's a bit confusing

@vjeux

vjeux Apr 7, 2017

Collaborator

Let's remove this comment, it's a bit confusing

src/printer.js
@@ -3331,6 +3333,13 @@ function printArrayItems(path, options, printPath, print) {
return concat(printedElements);
}
+function getJSXRawValue(node) {

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

This function name is not correct since you are also converting it to be a double quote. I would just inline those two lines above so you don't have to find a name for it.

@vjeux

vjeux Apr 7, 2017

Collaborator

This function name is not correct since you are also converting it to be a double quote. I would just inline those two lines above so you don't have to find a name for it.

src/printer.js
+function getJSXRawValue(node) {
+ const value = node.value.extra
+ ? node.value.extra.rawValue
+ : node.value.value

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

this should be node.value.raw

@vjeux

vjeux Apr 7, 2017

Collaborator

this should be node.value.raw

-<div id=\\"&quot;'<>&amp;quot;\\" />;
-<div id=\\"&quot;'<>&amp;quot;\\" />;
+<div id=\\"&quot;'<>&quot;\\" />;
+<div id=\\"&quot;'<>&quot;\\" />;

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

This is a regression! The &amp; shouldn't disappear

@vjeux

vjeux Apr 7, 2017

Collaborator

This is a regression! The &amp; shouldn't disappear

@@ -304,8 +317,8 @@ exports[`quotes.js 1`] = `
<div id='\\"&#39;<>&amp;quot;' />;

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

In jsx/jsfmt.spec.js can you add "babylon" to the list.

run_spec(__dirname, null, ["babylon", "typescript"]);
@vjeux

vjeux Apr 7, 2017

Collaborator

In jsx/jsfmt.spec.js can you add "babylon" to the list.

run_spec(__dirname, null, ["babylon", "typescript"]);
tests/jsx/html_escape.js
@@ -0,0 +1,4 @@
+// Should be preserved!

This comment has been minimized.

@vjeux

vjeux Apr 7, 2017

Collaborator

I would remove the comment, it's unclear what should be preserved.

@vjeux

vjeux Apr 7, 2017

Collaborator

I would remove the comment, it's unclear what should be preserved.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 7, 2017

Collaborator

Can you also make sure that you use raw JSX value for JSXText. See this issue: #1056

Collaborator

vjeux commented Apr 7, 2017

Can you also make sure that you use raw JSX value for JSXText. See this issue: #1056

yamafaktory added some commits Apr 8, 2017

Move getJSXRawValue function logic to the its only call, drop it.
A simple check is performed to determine if the parser is babylon or
flow via `n.value.extra`. Thus, the corresponding raw value is
extracted. If we are converting a string from single quotes to
double quotes, we need to make sure that double quotes are converted
to &quot;.
@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Apr 8, 2017

Contributor

@vjeux I've gone through all the points of your review and cleaned-up everything, this should be hopefully good to go now 🚀.

Contributor

yamafaktory commented Apr 8, 2017

@vjeux I've gone through all the points of your review and cleaned-up everything, this should be hopefully good to go now 🚀.

- res = '"' + util.htmlEscapeInsideDoubleQuote(n.value.value) + '"';
+ const value = n.value.extra ? n.value.extra.raw : n.value.raw;
+ res = '"' +
+ value.slice(1, value.length - 1).replace(/"/g, "&quot;") +

This comment has been minimized.

@vjeux

vjeux Apr 8, 2017

Collaborator

fyi, you can do .slice(1, -1)

@vjeux

vjeux Apr 8, 2017

Collaborator

fyi, you can do .slice(1, -1)

This comment has been minimized.

@yamafaktory

yamafaktory Apr 8, 2017

Contributor

Oh, yeah you're right!

@yamafaktory

yamafaktory Apr 8, 2017

Contributor

Oh, yeah you're right!

@vjeux vjeux merged commit d1191ad into prettier:master Apr 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 8, 2017

Collaborator

Thanks!

Collaborator

vjeux commented Apr 8, 2017

Thanks!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 8, 2017

Collaborator

Could you also do the same thing for htmlEscapeInsideAngleBracket. We want to be consistent, we should respect the encoding there as well.

Collaborator

vjeux commented Apr 8, 2017

Could you also do the same thing for htmlEscapeInsideAngleBracket. We want to be consistent, we should respect the encoding there as well.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Apr 8, 2017

Contributor

Sure! In another PR I guess? What do you think of adding a linting step in the test pipeline as another PR too?

Contributor

yamafaktory commented Apr 8, 2017

Sure! In another PR I guess? What do you think of adding a linting step in the test pipeline as another PR too?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 8, 2017

Collaborator

Yes, please another PR! What kind of linting step do you want?

Collaborator

vjeux commented Apr 8, 2017

Yes, please another PR! What kind of linting step do you want?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment