Add AMD Support

[daedalus28]

As promised in #17, here is the merge request. It makes the library behave like it does on node when loaded in an AMD environment.

[sgress454]

Thanks @daedalus28, testing now...

[sgress454]

@daedalus28 I guess I'm a little rusty with require.js. Can you post an example of how to use this to connect to Sails and make a socket request in an AMD environment?

[daedalus28]

Sure. It would look very much like it does in node, just wrapped in a define call. You'd require in socketio, sailsio, and call the sailsio function with socket io just like in node (which is the SailsIOClient from the source)

define(function(require) {
    var socketio = require('path/to/socketio');
    var SailsIOClient = require('path/to/sailsio');
    var io = SailsIOClient(socketio);
    io.sails.url = 'http:/example.com';

    io.socket.get('/example/path', { data: 'example' }, function(response) {
        console.log('got response', response)
    });
});

[daedalus28]

Note that is using the commonjs style syntax. You could do it a bit more traditionally like this:

define(['path/to/socketio', 'path/to/sailsio'], function(socketio, SailsIOClient)  {
    var io = SailsIOClient(socketio);
    io.sails.url = 'http:/example.com';

    io.socket.get('/example/path', { data: 'example' }, function(response) {
        console.log('got response', response)
    });
});

[sgress454]

Got it, thanks for that.

So the only issue I see here is that in order for it to continue to work as it always has in a non-AMD environment, we need to still have the socket.io code prepended to the top of the sails.io.js file. But since that code has its own define() call in it, it breaks yours. Fortunately, this repo includes its own copy of socket.io.min.js that it uses to create the dist version of sails.io.js, which means we can just remove the define() call from that code and make everything work. You should still be able to include socket.io.js separately when doing require. Do you see any problems with this?

[daedalus28]

I don't think modifying the bundled socktio client is the way to go. Bundling sails.io.js with the socketio client is less than ideal and adds additional maintenance keeping socketio up to date (especially since socketio is the only dependency this library has). The way I have it set up is to require the sails.io.js file from the root instead of the dist directory. In my app, I'm using socket.io-client v1.3.5 instead of the bundled 1.2.1 and I didn't have to modify anything to do so (it works great by the way - probably worth bumping the version if you're intent on keeping it in the repo). At a minimum, anyone using requirejs should just use the non-dist version as well. This is easy enough as installing with bower includes everything you need.

Also, while I understand wanting to make it easy for people to just drop in one file, it's fairly common practice to add dependencies first with standard script tags, like adding jquery to a page before plugins. It's a bit of a different scenario, but I can't imagine someone expecting a jquery plugin to come bundled with jquery. Adding the socketio client and then the sails.io.js file in two script tags for non AMD environments seems like an acceptable solution, so removing the bundled client may be the best option all around. But I understand that might not be something you're willing to change right now.

With all of that said, I have confirmed that this change continues to work as is without any errors in a non AMD environment. I spun up a fresh sails 0.11 site and patched the bundled sails.io.js to include my changes, and everything still worked just fine. This change should only affect AMD users in that they have to use the non dist version (and would actually break if they included it in the DOM after their AMD loader - at which point define would exit, triggering the AMD path and causing an error due to multiple anonymous define calls in one file), but I think this is probably the better route anyway.

I can add some notes about this to the readme in my pull request - AMD users should use the non dist version of the file, which is at the root when you do a bower install.

[sgress454]

It's not so much about people wanting to only drop in one file--it's that when you run sails new to create a new Sails project, it just installs the dist file in your assets folder. Most folks will never even see this repo.

We could update the installer (sails-generate-sails.io.js) to instead copy both socket.io.js and sails.io.js (without the bundled socket.io). Then we wouldn't need any notes for AMD users.

The reason we're using the older version of the socket.io client is just because it's been tested with sails.io.js; we're happy to upgrade to the newer version if it passes all the tests.

@mikermcneil, any thoughts on this? Did we have any other reasons to bundle the socket.io client with sails.io.js?

[daedalus28]

I just want to reiterate that this doesn't break anything for most users - only when using sails.io in a requirejs environment, loading it after requirejs, and using the sails.io.js file bundled with socket.io. And that's the exact situation that this change improves because we now have proper AMD support - all they'll need to do is use the non-bundled file.

@mikermcneil / @sgress454 What would it take to get this merged? I've been running my fork in production and things have been pretty smooth. I'm happy to do a pull request on sails-generate-sails.io.js as well.

[daedalus28]

Any updates yet? Should I update sails-generate-sails.io.js as well?

[sgress454]

Sorry, popped out of town for a few days.

I just want to reiterate that this doesn't break anything for most users

True, but not for the reason you're thinking. Most users will never see this code unless we change the generator. And if all we did was create a new dist version with the socket.io code prepended to your code, it wouldn't work for AMD users because of the define code that socket.io includes. Ideally we want one system for everyone, whether they want to use AMD or not. They should just be able to do sails new app and go on their merry way. In order for that to happen, we'd need to update the sails-generate-sails.io.js generator to output both your new version of sails.io.js (without the socket.io code) and a copy of socket.io.js, and also update the pipeline.js file in sails-generate-frontend so that the auto-generated layout file includes <script> tags for both, in the correct order.

That all does seem a bit involved. One alternative would be to keep the dist version as-is, but add a note to the to with a link to instructions for the AMD user.

[daedalus28]

Are we ok having the dist version be out of sync with the dev version? If so, that's fine by me and we should be able to include the merge as is.

Either way, I am happy to do a pull request on sails-generate-sails.io.js and sails-generate-frontend so we can remove the bundled socket.io client. Everything you mentioned seems very clear to me how to do with one exception being ensuring load order. I can add a bower inclusion like this one in sails-generate-sail.io.js and add a copy step over here in its generator. In the actual sails-generate-frontend code, it looks like it just calls the sails.io.js generator (see here). Would I have to do anything else? As long as the scripts are listed in the generator in the right order, it should be fine, right?

[daedalus28]

@sgress454 are we good to merge?

[sgress454]

Yes! What we'll do is leave the code in dist as-is, but add a note to the top that indicates how to go about using sails.io.js in an AMD environment (i.e. how to download the file from this repo). Thanks for your patience and your work on this!