gimite / web-socket-js

HTML5 Web Socket implementation powered by Flash
BSD 3-Clause "New" or "Revised" License
2.73k stars 489 forks source link

Only log debug output when we allow it. #78

Closed 3rd-Eden closed 13 years ago

3rd-Eden commented 13 years ago

Howdy,

This pull request silences all possible debug and logging output from the web_socket.js file unless the flag window.WEB_SOCKET_DEBUG has been set to true.

I have implemented this because allot of developers where getting confused and thought that console.error's where actually thrown errors. We have received various of bug reports and questions on both irc and stackoverflow about these.

So I thought it would be smart to hide them behind the debug flag as all these statement provide debugging information.

imanel commented 13 years ago

I believe that #79 will be better approach - if we do logging your way we will need to remember about "debug switch" every time we will use logger. Also - #79 approach will allow developer of library basing on web-socket-js to overwrite logger method and use his own.

3rd-Eden commented 13 years ago

I'm fine with #79 as well. As long there is way to silence the output.

gimite commented 13 years ago

I believe most of the message to console.error is fatal (non-recoverable) errors, so it's right that user thinks it's an error. One exception I can think of is that you want to further fallback to something else if the browser has neither WebSocket nor Flash Player >=10. In that case, it would be probably enough to add a flag to suppress "Flash Player >= 10.0.0 is required." message? Please let me know if you have some other motivation.

imanel commented 13 years ago

This issue was resolved by #79 so probably it can be closed.

Note: to redirect error to log you can use:

window.WEB_SOCKET_LOGGER = {
  log: function(msg) { console.log(msg) },
  error: function(msg) { console.log(msg) }
}

Note2: It would be wise to check if console exists as it is checked in current code. This example was stripped from this test for better readability.

3rd-Eden commented 13 years ago

@gimite

Sure in most cases it's nice to know that there are fatal errors, but only when you are developing. Once you deploy your code in production you really don't want to see any of it.

But as @imanel said, I can already silence all output with that patch.

EDIT nvm, that patch still makes it impossible, because it still doesn't expose away of silencing the output after the code has been loaded in to the page, only before. Which is in my case less than ideal.

gimite commented 13 years ago

Once you deploy your code in production you really don't want to see any of it.

You shouldn't deploy your code which outputs fatal errors :)

EDIT nvm, that patch still makes it impossible, because it still doesn't expose away of silencing the output after the code has been loaded in to the page, only before. Which is in my case less than ideal.

web_socket.js can output something to console.error while loading. So if we allow changing the logger after loading, you cannot silence everything by that. And I believe it's not difficult to write code to set logger before loading web_socket.js. Please let me know if there's a case when it's difficult.