search

liferay / liferay-npm-tools

Collection of tools for using npm in Liferay
Other
18 stars 15 forks source link

JSP formatter may strip necessary quotes from object properties #300

Closed wincent closed 4 years ago

wincent commented 4 years ago

~We don't have any examples of this in liferay-portal right now — you can verify this with git grep "'<%=.+%>':" -- '*.jsp' — but~ @victorg1991 just came and showed me one from a change that he is currently preparing. Basically, he has a JSP expression that is being used in a JS object key:

(Update: actually, we do have examples of this pattern, but they were all removed in 916ef8315ba07, and you can see them with git grep "'<%=.+%>':" 916ef8315ba07~ -- '*.jsp'; will need to audit whether any of those are problematic.)

{
    '<%= value %>': true
}

In order to pass template text into Prettier, we have to turn it into valid JS, so we substitute all expressions:

{
    'ʾJSP_EXPR___': true
}

Note that at the time we do this substitution, we don't have a valid JS AST yet, just raw text, which is why we can't "know" that the expression is inside a JS string, and that in this case we could get away without touching it. As a result, Prettier will strip the apostrophes, which it considers unnecessary:

{
    ʾJSP_EXPR___: true
}

And when we round-trip that back to JSP, we get:

{
    <%= value %>: true
}

And at run-time, that can produce an invalid result; for example, if value is actually "foo-bar-baz", the result JS will be:

{
    foo-bar-baz: true
}

which is invalid, unlike the desired:

{
    'foo-bar-baz': true
}

There are at least a couple of workarounds. One is to use a computed property name, which Prettier will leave untouched:

{
    ['<%= value %>']: true
}

The problem with that, however, is I believe that IE will choke on it. So, we have to go with this instead:

{
    // prettier-ignore
    '<%= value %>': true
}

~If this comes up again, we might have to come up with a disgusting hack to special-case this scenario, but until it does I suggest we do nothing, which is why I am documenting it here.~ (Update: see my edit at the top — we need to check that the quoted strings removed in 916ef8315ba07 are actually ok.) In the long-term, we want to get rid of all non-trivial JS from JSP anyway, so hopefully we won't see this again.

wincent commented 4 years ago

Or, I should mention another workaround, in case the // prettier-ignore suppression is too ugly. You can create a variable and assign the property to that instead:

var object = {};

object['<%= value %>'] = true;

Seems like Prettier will leave that untouched too.

wincent commented 4 years ago

Two more workarounds to mention:

wincent commented 4 years ago

Ok, going to close this now that it is documented and hopefully dealt with:

@victorg1991 sent https://github.com/brianchandotcom/liferay-portal/pull/80065 to fix the syntax error, and I did an audit of the rest of the repo to see if there were any other potential issues; I'm testing that current at https://github.com/wincent/liferay-portal/pull/85 and if it looks good I'll forward it on upstream.

Details of the audit:

Full list of sites to inspect in this gist (probably easier to read in raw format).