replace triple mustache with html-safe helper #1881
Conversation
|
r? @sgrif (rust_highfive has picked a reviewer for you, use r? to override) |
|
What is the goal of this change? There is another occurrence of triple braces here; should that be replaced as well? |
|
I have replaced the other occurrence :) The idea is that we should move away from triple mustaches. Since we still need to interpolate some HTML–which we kinda control anyway–the general advice is to use the |
|
I believe this is a lint starting with some version of ember.js, hence my change. |
locks
force-pushed
the
triple-stache
branch
3 times, most recently
from
3e828a6 to
8dbc20c
Compare
smarnach
left a comment
smarnach
left a comment
There was a problem hiding this comment.
Sorry for posting so many comments on such a simple PR. I'm not really a frontend person, so I need a bit of help to understand what's going on here – I'd like to understand the change before approving it. :)
app/templates/crate/owners.hbs
Outdated
| <div class='stats'> | ||
| {{{ if user.email user.email " " }}} | ||
| {{#if user.email}} | ||
| {{html-safe user.email}} |
There was a problem hiding this comment.
Does the user email really need html-safe? I believe the triple braces were only there to prevent from being escaped, and if you expand it the way you did we don't need it anymore.
There was a problem hiding this comment.
Hm, good point. I tried to keep the exact same behaviour as I wasn't sure, but if we don't need it here then even better!
There was a problem hiding this comment.
Thinking about it, marking the email address as "safe" even appears to be actively harmful, and we don't do so in other places we show email addresses. This currently doesn't actually introduce an XSS vulnerability since the only email address we show is the address of the current user, so people could only attack themselves, but I'd prefer if you remove html-safe here.
| import { helper } from '@ember/component/helper'; | ||
| import { htmlSafe as markAsSafe } from '@ember/template'; | ||
|
|
||
| export function htmlSafe([content] /*, hash*/) { |
There was a problem hiding this comment.
Can we use array destructuring on all browsers we support? Or do we compile the JS files somehow?
There was a problem hiding this comment.
config/targets.js tells ember-cli-babel which browsers we support so it knows which features to compile, and I think our listed browsers support array destructuring. I'd have to check UCAndroid.
There was a problem hiding this comment.
Ah, the JS files are compiled with Babel, that's all I need to know. I don't know our frontend infrastructure too well.
No worries, that's what reviews are for 😁 it also helps me make sure I'm aligned. |
|
Thanks for the updates, looks good to me now! @bors r+ |
|
📌 Commit 5e935ca has been approved by |
replace triple mustache with html-safe helper
|
☀️ Test successful - checks-travis |
No description provided.