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

Add WebSocketLogger and use it in every submodule. #79

Closed imanel closed 13 years ago

imanel commented 13 years ago

This allow to easly manage debug switch, but also allow overwrite of logger for use in other libraries using web-socket-js. I believe that this is much better approach than #78 - there will be no need to remember about adding "debug" check each time, and also user will be able to write his own logger(i.e. disabling 'log' and allowing only 'error', or formatting log for his own need)

imanel commented 13 years ago

One note - I had no idea how to name or namespace this logger, so if anyone have good idea then please post it here and I will rename WebSocketLogger to something more friendly and add to this pull request.

3rd-Eden commented 13 years ago

@imanel I would namespace it under WebSocket like all web-socket-js methods are namespaced. I see no reason to introduce another global variable for this.

WebSocket.logger.log or something like that would do fine, as it can easily be overwritten by third party and it doesn't leak any globals.

imanel commented 13 years ago

It's because when you write app you don't know if window.WebSocket is available or not. If not, and web-socket-js is not loaded yet, then window.WebSocket.logger (example name*) would return noMethodError(WebSocket). That's why we need to set it outside of window.WebSocket

3rd-Eden commented 13 years ago

@imanel You can feature detect the existence of the fallback using 'WebSocket' in window && '__initialize' in WebSocket. So I don't really see a need of polluting the browser with another global.

But what you could do is:

Websocket.logger = window.WebSocketLogger || {log:.. error: .. }

Which provides the best of both worlds, no unnecessary globals, and override it needed.

imanel commented 13 years ago

There are 3 situations:

1) Browser support WebSocket OOTB. You have WS global, but no logger. You will do WebSocket.logger = [something]

2) Browser doesn't support WS, but fallback is loaded. You will do WebSocket.logger = [something]

3) Browser doesn't support WS, and fallback is not loaded. You will need to define WebSocket and set WebSocket.logger = [something], but then fallback is loaded, detects that window.WebSocket != 'undefined' and will NOT load. You can alwasy NOT define WebSocket - then fallback will load and run, but your logger will be disabled.

That's why both window.WEB_SOCKET_DEBUG and window.WebSocketLogger will need to be outside of window.WebSocket. Question is about name "WebSocketLogger" and not about if it should be global or not - because there are no other way except of global.

Alternatively we could create one global to rule them all(debug/logger/swf location) This will be probably best idea, but I'm not sure if it's easiest to write. Thanks to that user will just need to define one global instead of 3:

window.WebSocketFallback = {
  debug: false,
  swf_location: 'some path',
  logger: function() { … }
}

If some of them will not be provided then we need to use default. BUT if fallback will load before user script, and he will overwrite default with his own, without some keys(like without logger) then we will need still fallback to default. Anyone want to write that?

3rd-Eden commented 13 years ago
  1. If a browser supports WebSockets out of the box, it wouldn't even need a special logger. And it wouldn't even be attached anyways as on line 8 there is a return false.
  2. Yes, if there isn't a native WebSocket implementation you probably want a logger to silence of format the output.
  3. You wouldn't need to define WebSocket && WebSocket.logger your self, you would just add a global for example WebSocketLogger and once the fallback is loaded it will check if that global exists and use that as a logger or it would just use it's own logger implementation.
// if window.WebSocketLogger is undefined it will use the {log, error} stuff.
WebSocket.logger = window.WebSocketLogger || {log:.. error: .. }

But having one uber global would be more than ideal. Less pollution and easier to manage. @gimite what is your opinion on this matter?

gimite commented 13 years ago

What's example of your use case of this? Messages to console.log and console.error is intended to developers (who can see console.log) and I don't think it's something you want to show to end users. To show only errors, you can set WEB_SOCKET_DEBUG = false;

imanel commented 13 years ago

Way to change logger is pretty easy example for me. I'm using web-socket-js in my Socky project and usually don't want to dusturb user with several different messages. Every message generated by Socky is namespaced under

Socky :: [namespace] :: [message]

So I would gladly namespace flash fallback messages under "fallback" and leave everything else untouched. Thanks to that user will not be confused "from where this message came?"

gimite commented 13 years ago

I see. I pulled your request. I renamed WebSocketLogger to WEB_SOCKET_LOGGER to be consistent with other global variables. I also changed default to output to window.console, because it should be useful for people who directly use web-socket-js. You can still output nothing by setting no-op logger to WEB_SOCKET_LOGGER.

I didn't understand well about @3rd-Eden 's suggestion. In the end it looks your suggestion is to use global variable window.WebSocketLogger, so I guess it's the same as @imanel 's implementation? Please let me know if you have better idea.

imanel commented 13 years ago

Thanks. I have one question-you have rewritten using logger that it's currently impossible to overwrite it after web-socket-js is initialized. It may be my missunderstanding, but is there any way to be sure that other javascript files will be loaded before web-socket-js? So if I overwrite WEB_SOCKET_LOGGER in my js, and fallback was loaded before it, then overwrite will do anything?

gimite commented 13 years ago

You need to set WEB_SOCKET_LOGGER before loading web_socket.js. If you put script tag with your code before script tag for web_socket.js, I believe it is executed before loading web_socket.js:

<script type="text/javascript">
  WEB_SOCKET_LOGGER = ...;
</script>
<script type="text/javascript" src="web_socket.js"></script>
imanel commented 13 years ago

And if I cannot guarantee that? That's why I tried to implement it in way that will allow for late changes. But I think that it's small problem-I will try to bypass it when testing unified namespace-we'll see if it's easy to implement without ugly code...

gimite commented 13 years ago

I believe it's guaranteed that script tags are executed in their order. Have you encounter situation where it doesn't? Anyway web_socket.js can output something to logger.error while loading, so setting logger after loading web_socket.js wouldn't work perfectly.