add Server.attach method #213

Merged
merged 5 commits into from Jun 4, 2014

2 participants

@defunctzombie

This method does what engine.attach use to do but is now on the
instantiated server instance. This makes it possible to create engine.io
servers without yet attaching them to any active http server.

When creating a project which will server static files/pages alongside
engine.io on the same domain/process this makes it easier to separate
logic into files without having to pass around the http server.

@rauchg

Docs plz

@defunctzombie

Where do the docs live?

@rauchg rauchg commented on the diff Feb 2, 2014
README.md
@@ -134,12 +134,9 @@ These are exposed by `require('engine.io')`:
- `http.Server`: server to attach to.
@rauchg
rauchg added a note Feb 2, 2014

This would be optional. We need to document the various ways this object can be instantiated.

How is this optional for the attach method? If you don't provide something to attach to... then attach can't do anything.

@rauchg
rauchg added a note Feb 2, 2014

I mean for constructing.

Yea, But that would be documented under the Server constructor docs right? For this attach call it is required. At least that is how the code currently is.

@rauchg
rauchg added a note Feb 3, 2014

Right I thought I was commenting on the ctor docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@defunctzombie

Added some more documentation about this to the readme as well as which methods I felt are [Recommended] vs which are not.

@rauchg
@defunctzombie
@rauchg

That's a really good point. In this case I think there's something to be said about the convenience of being able to require('engine.io')(server) and be done with things tho.

@rauchg

Like you don't need a reference to the module itself, you just want the io isntance with the attached http server.

@defunctzombie
@rauchg

We need to update the README to use the shorter syntax whenever possible.

@defunctzombie

@guille which syntax? `require('engine.io')(http_server) ?

I like the var eio = require('engine.io')(); eio.attach(http_server) approach for the flexibility it provides in having a server object before attaching.

Let me know what format is desired and I will update the readme.

@defunctzombie

Is there still something to do on this PR?

@rauchg

Yes. We gotta remove the "recommendations" from the README.

@defunctzombie

Updated the PR. Removed the Recommended words.

@rase- rase- and 1 other commented on an outdated diff Jun 4, 2014
+- `handleSocket`
+ - Called with raw TCP sockets from http requests to intercept flash policy
+ file requests
+ - **Parameters**
+ - `net.Stream`: TCP socket on which requests are listened
+ - **Returns** `Server` for chaining
+- `attach`
+ - Attach this Server instance to an `http.Server`
+ - Captures `upgrade` requests for a `http.Server`. In other words, makes
+ a regular http.Server WebSocket-compatible.
+ - **Parameters**
+ - `http.Server`: server to attach to.
+ - `Object`: optional, options object
+ - **Options**
+ - `path` (`String`): name of the path to capture (`/engine.io`).
+ - `policyFile` (`Boolean`): whether to handle policy file requests (`true`)
@rase-
rase- added a note Jun 4, 2014

policyFile isn't here anymore, is it?

good catch, fixed and removed the handleSocket docs as well since that no longer exists

@rase-
rase- added a note Jun 4, 2014

Perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
defunctzombie added some commits Jan 19, 2014
@defunctzombie defunctzombie add Server.attach method
This method does what engine.attach use to do but is now on the
instantiated server instance. This makes it possible to create engine.io
servers without yet attaching them to any active http server.

When creating a project which will server static files/pages alongside
engine.io on the same domain/process this makes it easier to separate
logic into files without having to pass around the http server.
d74f93e
@defunctzombie defunctzombie remove unused require d3ed841
@defunctzombie defunctzombie remove lib/index.js
Needless level of indirection which blocks actually reading the
important parts of the code.
cb2b80a
@defunctzombie defunctzombie make require('engine.io')() return a new Server instance
fixes #212
48f006f
@defunctzombie defunctzombie lib: remove unused file b836605
@rauchg rauchg merged commit 2ec3d89 into socketio:master Jun 4, 2014
@rauchg

Huge simplification. Thanks @defunctzombie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment