Console warning #11625
Console warning #11625
Conversation
Added warning to console.
lancewillett
added
the
[Status] Needs Review
|
Not all browsers support color input. We should determine if the console supports color and print according. We should i18nize the message. I guess for added security, we could run the console call in a setTimeout or try-catch so any error doesn't prevent the rest of the execution. What do you think @mtias ? |
umurkontaci
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
|
@tfroseman are you still interested in pushing out this PR? |
|
awesome - thanks @tfroseman! yeah I would suggest going ahead and addressing what @umurkontaci raised. @umurkontaci do we have numbers on that behavior for likewise I'm curious about translation since this is just an eval'd JS string in the HTML header. if we wanted to translate it seems like we would need a lot more code (maybe it belongs in the app and not the HTML header…?) |
I think IE < 10 doesn't have console before you open the dev tools, it could be the same in IE > 10 as well. I don't have any data on how browsers behave regarding colored console commands when they don't support it. But this will execute very early and on every load, so we can't have any errors thrown. Although I think it might be a better to fit to run this after the app is loaded. That way we'll have access to translation module and error capturing. |
Added try catch to log errors, and check for console.
server/pages/index.jade
Outdated
| } | ||
| } catch (exception) { | ||
| // Do something with error | ||
| } |
There was a problem hiding this comment.
why the try/catch if we are also checking for the presence of window.console? seems like we would only need one or the other
There was a problem hiding this comment.
Made sense at the time as a way to meet the the need for error capturing if needed this early in app loading. Not really sure what errors could/would get thrown from writing to console.
|
@tfroseman what's the status on this PR? would you like to continue to refine it or have you moved on? are you waiting on feedback from one of us? |
|
@dmsnell Wasn't sure if the ability to support localization would fall under the scope of this issue. With that said I would like to have this added, I'll remove the redundant check. |
| } | ||
| console.log("If someone told you to type or paste something into this area it could compromise the security of your account."); | ||
|
|
||
| } |
There was a problem hiding this comment.
@tfroseman do you think you could fix the formatting issues here? I think that if we get that updated we can ship. We will also prefer single quotes on the strings.
There was a problem hiding this comment.
@dmsnell Yeah I'll clean that up. Also I was testing console on safari and realized that it and IE do not support 'rich' console output. Can I just do agent sniffing and only output plain text to log?
There was a problem hiding this comment.
Seemed to work in Safari. What specific troubles did you find there? We'd probably prefer not to try and sniff browser agent.
If it's a big problem we can probably start by leaving it all black.
There was a problem hiding this comment.
The css styled output.
I am on Version 10.0.1 (11602.2.14.0.7) if that makes any difference.
Edit: I had to select the view all filter within developer tools.
There was a problem hiding this comment.
I'm on 10.1.1 and that's working. I'm fine leaving it in for Safari. IE is the bigger question.
|
@dmsnell You are correct works on everything but IE as it does not yet support css in console. |
|
Resolved with the merge of #18088 |
Added warning to console. Fixes #15
