Fully backwards compatible patch (not breaking current implementations): 1. manual init of now.js on client side, 2. client side setting of host and port 3. self-host socket.io 4. custom client side socket.io options 5. connect_failed event

tommedema commented 13 years ago

This is a fully backwards compatible patch which introduces 5 great improvements (if I may say so myself) to the client-side of now.js, as it is now allowing:

manual init of now.js on client side;
client side setting of host and port;
self-hosting of socket.io by manually setting dependencies;
custom client side socket.io options;
listening to connect_failed event.

These changes are completely backwards compatible and do not change any current behavior or implementations.

A dev can now manually host socket.io and other dependencies, by manually initializing now.js:

<head>
    <script type='text/javascript'>
        __NOWMANUALINIT = true;
    </script>
    <script type='text/javascript' src='/lib/now.js'></script>
    <script type='text/javascript'>
        now.core.init('localhost', 3000, null, {
            transports : ['websocket', 'flashsocket', 'htmlfile']
        });
    </script>
</head>

This should not break any current implementations. I would consider the above approach a must for more advanced applications.

The advanced init parameters: function(host, port, dependencies, socketOptions). None of these are required, as unset values will default to the previous implementations.

Thus, all that needs to be done to use this more advanced approach is to set __NOWMANUALINIT = true before including now.js.

To clarify, this still works too:

<head>
    <script type='text/javascript' src='/nowjs/now.js'></script>
</head>

And everything will go automatic, using the server-generated now library.

Finally, it should be possible to detect a failed connection now (please check):

now.core.on('connect_failed', function() {
    //inform the user that we cannot make a connection with the available transport types
});

tommedema commented 13 years ago

Also, this patch allows lazy loading of NowJS. Eg. if you have a javascript app where only at a certain stage such real time connection is needed, nowjs can be initialized when this stage is entered.

sridatta commented 13 years ago

I've only taken a cursory glance but this looks pretty useful. Preserves the "magic" for simple use cases but gives more fine-tuned control for more serious developers.

I'll look at the code some more when I get the time. Thanks, Tom.

AD7six commented 13 years ago

Of your 5 points

1 - I assume it was an anonymous function for a reason - why wouldn't you lazy load now if you want to lazy use it? Meaning

$.get("/nowjs/now.js", function() {
    now.ready(function() {
    });
});

I can see the benefit of having the code already available, I just want to know your answer.

2,3,4 - you can already do that if you load socket.io yourself (either lazily or in the page source), and putting that logic in now I don't think is a good idea (it adds complexity it /may/ be useful to someone but less than 20%, and they can achieve the same goal already). In addition, if there were 2 dependencies your changes would apply to both - would you send a pull request in the future to allow each dependency to be loaded from a different host?

5 - If socket.io has this event, cool.

IMO

AD PS. several small atomic (individual) pull requests would be easier for the project to integrate - you've suggested 3 different things which means they can't merge your pull request automatically/easily if any of those changes are not accepted.

tommedema commented 13 years ago

Hi AD,

Thanks for your input.

1a. while this may seem easy using your jQuery abstraction wrapper, it's actually quite a complex task to load a script in Javascript with AJAX in a cross-browser way 1b. in the future, when now.core.disconnect is added, this also allows intermediate disconnection, for example in a staged application, which allows connecting to the server, do some work and disconnect again until another live session is needed in a later state (at which one can call now.core.init again).

2,3,4a. it doesn't really add complexity, since you only need to do this if you are having this need for a more complex initialization (at which point you cannot get around slightly more complexity) - normal users can make it auto-init like it does by default

2,3,4b. that '20%' (I believe it's more by the way) cannot achieve the same goal already, because socket.io cannot be loaded manually as this is hardcoded inside now.js - in fact, this patch does allow you to load socket.io manually

2,3,4c. at the moment there is no support for letting each dependency load from a different host, this patch does not change that. Also, that behavior would be quite rare. If you are doing advanced enough work to have multiple nodes with high availability etc., you'd generally use a CDN, and if not, still access static files from the same place that is optimized for static assets

I guess the reason I didn't make several pull requests was because all these new features (except the event) are dependent of each other. Eg., no dependencies can be set if now.js cannot be initialized manually.

Tom

AD7six commented 13 years ago

1a. while this may seem easy using your jQuery abstraction wrapper, it's actually quite a complex task to load a script in Javascript with AJAX in a cross-browser way

You're saying https://github.com/Flotype/now/blob/master/lib/now.js#L418 is complex (I used jquery for brevity)

2,3,4 I'm not talking about userland code - I'm talking about the now.js source (and I think you haven't understood what the recent dependency loading changes mean)

I don't see how lazy-starting now, loading dependencies from a different host and a new socket.io event are related.

AD

tommedema commented 13 years ago

Much more complex than now.core.init ;) And again, there is no use in this since now.js does not allow you to initialize socket.io by yourself, until this patch.

Apart from the event they are related, depedencies are loading during initialization. If you cannot initialize now manually, you cannot set these dependencies either. However, much more important than dependencies is the socketOptions parameter.

AD7six commented 13 years ago

much more important than dependencies is the socketOptions parameter.

That change is buried among other unrelated changes.

You're right that's not possible right now. But why would you want to change that on a client by client basis (which is what you're permitting) instead of modifying how now serves that file so that the options (defined server side) are embedded in the source (an example).

AD

tommedema commented 13 years ago

There's an easy answer to that, many of us do not like the way now is serving this file automatically. There are numerous reasons:

outsourcing to CDN
bundling client-side JS to minify (eg. google closure's advanced compilation options requires all code to be bundled as otherwise references inbetween code will fail) and gzip
from a conceptual point of view, it makes much more sense to set server address and host on the client-side rather than the server-side
setting it per client allows you to set it dynamically, eg. make the first X allocated clients gather from source A while the latter Y clients gather from source B, in short it allows for a lot more possibilities

AD7six commented 13 years ago

Again I don't see how any of that means now should change how it loads socket.io (in the same way as the ticket description for one of my tickets which suggested this was wrong).

right now you can load socket.io however you want, so long as you do so before loading now. Putting logic in now.js for loading socket.io from somewhere else is imo wrong - userland code does that, and now just takes care of the simplest case (making sure no matter what, that it works).

tommedema commented 13 years ago

Hi AD,

I'm afraid that is simply wrong. What now.js does, is load '/socket/socket.io.js' - which will cause socket.io's selfhosting mechanics to trigger. Other than making this request, it will also start a download, waist precious loading time and bandwidth.

If you want to selfhost socket.io, you have to make sure that request is never made, only then can you efficiently host it at a location like '/lib/socket.io.js'.

Anyway, we'll see what "the others" think.

AD7six commented 13 years ago

I'm afraid that is simply wrong. What now.js does, is load '/socket/socket.io.js'' - which will cause socket.io's selfhosting mechanics to trigger

now.js only tries to load socket.io at all if it's not already loaded (which was the whole point of #95, already merged to master)

dvv commented 13 years ago

@AD7six: frankly, truthy window.io is not the 100% sign of socket.io.js loaded, and document.createElement('script') is not 100% cross-browser script loader (if it were, we wouldn't ever need all bouquet of loaders...)

The autorun closure at the end of now.js can be really annoying, as it completely punts on the way you (as particular developer) do want your init process should run. Things settle if we have means to inhibit auto-running this closure. The idea of the patch is that simple.

AD7six commented 13 years ago

@AD7six: frankly, truthy window.io is not the 100% sign of socket.io.js loaded,

Very true, but is it sufficiently correct for now.js' dependency loading logic?

and document.createElement('script') is not 100% cross-browser script loader (if it were, we wouldn't ever need all bouquet of loaders...)

So you're also saying that now.js is not 100% cross-browser compatible since that is exactly how it loads dependencies (even with this patch)?

The autorun closure at the end of now.js can be really annoying, as it completely punts on the way you (as particular developer) do want your init process should run. Things settle if we have means to inhibit auto-running this closure. The idea of the patch is that simple.

I'm personally all for an explicit init method and removing the auto-connect nature of now

AD

dvv commented 13 years ago

As NowJS tries to hide dependency on socket.io (which is imho why autorun is done), window.io is not reserved as the very special key, and any user (presumably noobs, again, for whom autorun is done ;) ) is free to set/override it. If it happens before now.js is Githubissues.

Githubissues is a development platform for aggregating issues.

Flotype / now

Fully backwards compatible patch (not breaking current implementations): 1. manual init of now.js on client side, 2. client side setting of host and port 3. self-host socket.io 4. custom client side socket.io options 5. connect_failed event #98