Why do you override error handling?

[swagger-api]

Why do you override error handling?

[eddieajau]

I think the problem is that Swagger isn't designed around the regular Express middleware paradigm (of course, finding documentation about that for Express is also challenge). Should we not be using something like app.use(swagger) instead of swagger.setAppHandler(app)?

[timruffles]

IMO it'd be lovely to design in a way that's composable and uses the tools express provides :). Then it can be used everywhere, with any express setup.

For example, routes can easily be handled using express.Router:

var swagger = new Swagger();
// since we're in swagger land, can use an extended API
swagger.putResource({ /* .. */ });

// now we're ready to apply it to an app instance
var app = express();
// generate and return an express.Router instance from our config
app.use(swagger.routes())

[timruffles]

👍 👍

Also - yes the connect/express middleware docs are really bad.

[jsdevel]

@eddieajau I see this module not so much as middleware but more as a wrapper around express apps.

With that in mind, I'd like to change the interface in such a way that it requires the appHandler on creation I.E.

var express = require('express');
var Swagger = require('swagger-node-express');
var app = express();
var mySwagger = new Swagger(app);

mySwagger.addModles([/*...*/]);
mySwagger.addResources([/*...*/]);

I'd also like to completely remove error handling logic as it should be defined on your application anyway.

Going with this approach would also solve versioning and probably many others. We could then do something like this:

var v1Swagger = new Swagger(app, {prefix: '/v1'});
var v2Swagger = new Swagger(app, {prefix: '/v2'});

v1Swagger.addModels([/*These are now isolated from v2*/]);
v2Swagger.addModels([/*These are now isolated from v1*/]);

cc @fehguy @enterline @therealklanni @tracker1

[rabchev]

@jsdevel +1

I completely agree with you. Multiple Swagger instances should make things much cleaner, especially when handling multiple API versions.

[jsdevel]

Thanks @rabchev! Luckily @enterline got us most of the way there with 9246582. A lot of the (anti-)?patterns (like error handling and headers etc.) were added prior to his commit. I feel as you do that being clean is pretty important at this point. We've been using swagger a lot and loving it! However, the code is still a bit scrappy and can be cleaned up even more. Cleaning it up will cause breaking changes, but perhaps @fehguy can weigh in on what the appetite is around here for doing that.

[fehguy]

Love the input and cleaning up this library. @jsdevel we can create a new minor version (2.1) if we break the API

[jsdevel]

Sounds great @fehguy! Thanks for replying. Shall we continue to make the PRs against master?

[fehguy]

@jsdevel i rebased the develop branch--let's use that for breaking changes.

[timruffles]

I vote for @eddieajau's approach: var swaggerInstance = swagger(); app.use(swaggerInstance). It's the node/express/'plays well with others' way. Wrapping express is actively anti-modular.

Also breaking API changes = major version change surely?

[joscas]

+1 @eddieajau. I think Swagger should work better with Express

[greenimpala]

+1 on getting this sorted.

[jsdevel]

@timruffles @eddieajau I disagree with "swagger should be middleware". Much of what swagger does is register operations (PUT, GET etc.). If swagger were middelware, it would have to handle all that manually. Correct me if I'm wrong.

I see swagger as more of an "app wrapper". var swagger = new Swagger(app);. To me this is perfectly acceptable, allows swagger to fully leverage express, and isn't too far from an acceptable look and feel.

[jsdevel]

@timruffles I'm not sure I see how that's much different from this:

var app = express();
var swagger = new Swagger(app);
swagger.putResource({/*..*/});

This way is terser and leverages express just fine. As middleware, swagger would only be given the req, res, and next objects. Wouldn't it then have to manually parse req.path to know which action to take?

[timruffles]

It wouldn't have to be middleware. As said, I recommend using generating an express.Router instance from the swagger config. That way you get all the access you want, but leave how to apply the routes to the express app up to the library user.

[jsdevel]

Internally, I believe swagger calls app.<verb> which appears to accomplish what using express.Router does.

[timruffles]

Right - but that was it's hidden from library uses, so it can't compose with another library. e.g take swagger's router, pass it into a another library that works with routers, etc etc.

I can't see any downsides to playing nicely with the express ecosystem.

[hillct]

I can't see how anyone could oppose playing better with the ecosystem but it seems to me that it would drastically expand the addressable userbase if swagger-node-express could be pushed down the stack to Connect, eliminating the dependency on Express, which is already a very thin layer offering what amounts to only a bit of user level sugar on top of the core stack functionality. I recognize it would require slightly more robust dependency management but it would immediately facilitate far more robust and modular API assembly along the lines of things like Connect-Architect

[jsdevel]

@hillct I think we're stuck with express given the name of this module.

[hillct]

As entertaining as that is, it's not a good reason to forego the significant advantage that pushing swagger down the stack really offers.

[jsdevel]

@hillct if I'm not mistaken, express 4.0 has completely removed connect as a dependency. I agree in principle with keeping things modular and reusable. I fail to see how the changes I've proposed break that. There are several modules out there already that are meant to be used with express and not connect.

[fehguy]

@jsdevel @hillct I do think you are both making very valid points. Swagger for node could become much lighter and portable with some refactoring.

As @jsdevel has pointed out, this particular implementation is geared towards express with the hope that it provides a very fast, low-configuration mechanism to integrate with Swagger. I would love to see this get forked or started as a pure "swagger-connect" middleware project.

Does it make sense to fork this project to satisfy that goal?

[jsdevel]

@fehguy that makes sense to me.

I just opened #131 to remove the error handling. Test included.

[jsdevel]

@timruffles see #131. What are your thoughts on that approach? It moves more towards leveraging express which I believe is in line with the desires expressed herein.

[hillct]

@jsdevel as you say, Express has 'removed' Connect as a dependency but it's done so by establishing a full middleware support layer that's essentially a duplicate of Connect such that what were formerly Connect-compatible middleware plugins are now Connect/Express compatible. I should have been clearer, that I'm not arguing against the changes you propose. I'm arguing that a relatively small amount of additional refactoring would yield a module that would be compatible with both Express and Connect.

Also, I completely agree with your sentiments in #130 (comment) as this would facilitate broadening supported stack configurations with which swagger-node-express could be deployed.

[jsdevel]

Thanks @hillct. Can you review #131?