-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for synchronous combination #5
Comments
I think introducing a "synchronous mode" would not comply with the core principles of node.js. However I understand the issue and agree that it should be possible to initialize Swagger Combine during application startup. If you are using node.js >= 7.6 you could use async/await to achieve this by creating a function which asynchronously returns a middleware function: const app = require('express')();
const { SwaggerCombine } = require('swagger-combine');
function middlewareAsync(config) {
return new SwaggerCombine(config)
.combine()
.then(sc => {
return (req,res, next) => {
res.json(sc.combinedSchema);
}
});
}
const config = {
swagger: '2.0',
info: {
title: 'Async Middleware Example',
version: '1.0.0'
},
apis: [{ url: 'http://petstore.swagger.io/v2/swagger.json' }],
};
(async function() {
try {
app.get('/swagger.json', await middlewareAsync(config));
} catch (e) {
console.error(e);
}
app.listen(3000);
})(); Instead of async/await you can also use other modules like co: const app = require('express')();
const co = require('co');
const { SwaggerCombine } = require('swagger-combine');
function middlewareAsync(config) {
return new SwaggerCombine(config)
.combine()
.then(sc => {
return (req,res, next) => {
res.json(sc.combinedSchema);
}
});
}
const config = {
swagger: '2.0',
info: {
title: 'Async Middleware Example',
version: '1.0.0'
},
apis: [{ url: 'http://petstore.swagger.io/v2/swagger.json' }],
};
co(function* () {
try {
app.get('/swagger.json', yield middlewareAsync(config));
} catch (e) {
console.error(e);
}
app.listen(3000);
}); In both of these examples To make this more convenient, we will implement the |
@fabsrc Thank you for the detailed response! I am somewhat puzzled by "core principles of Node.js" statement, though - Node.js itself provides both sync and async versions for e. g. file operations since there are many different cases, not all of which benefit from async nature. |
@kibertoad I was referring to the event-driven nature of node.js (including asynchronous I/O). You are right, there are some use cases where synchronous file operations make sense (e.g. during application startup or in CLI tools) however as far as I know there is no "native" way of making HTTP requests synchronously in node.js which would be required to introduce a "synchronous mode" in the Swagger Combine module. |
@fabsrc You don't need to make an HTTP request to use SwaggerCombine, actually - you can always point to a local JSON instead which can, indeed, be loaded synchronously. This is a pretty typical usecase - application has split jsons in its sources and wants to serve them as a single combined one. (it already works, btw, but asynchronously) |
@kibertoad Yes for local files it would work synchronously, but this module internally uses the Swagger Parser module which does all the resolving and dereferencing of the Swagger schemas. To allow both URLs and files to be used, this is done asynchronously. A "synchronous mode" for Swagger Combine would therefore require a "synchronous mode" in Swagger Parser. |
It seems like a synchronous API is currently being implemented by the creator of Swagger Parser (see APIDevTools/swagger-parser#54 and APIDevTools/json-schema-ref-parser#14). |
Current implementation makes it difficult to combine swagger during application startup, as e. g. you cannot add middleware to Express application within an asynchronous block, since it will already finish initialization by then.
Would it be possible to introduct optional synchronous mode as well? It's not a big deal if it will block entire application, it's not a problem during the startup phase.
The text was updated successfully, but these errors were encountered: