Skip to content
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

Open
kibertoad opened this issue Sep 23, 2017 · 6 comments
Open

Support for synchronous combination #5

kibertoad opened this issue Sep 23, 2017 · 6 comments

Comments

@kibertoad
Copy link

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.

@fabsrc
Copy link
Contributor

fabsrc commented Sep 25, 2017

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 app.listen(3000) is only invoked after Swagger Combine is initialized, which is, as far as I understood, the behavior you described.

To make this more convenient, we will implement the middlewareAsync function in the Swagger Combine module soon.

@kibertoad
Copy link
Author

@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.

@fabsrc
Copy link
Contributor

fabsrc commented Sep 25, 2017

@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.

@kibertoad
Copy link
Author

@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)

@fabsrc
Copy link
Contributor

fabsrc commented Sep 25, 2017

@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.

@fabsrc
Copy link
Contributor

fabsrc commented Oct 4, 2017

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).
Once this is done, we will make it available in the Swagger Combine module as well.

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

No branches or pull requests

2 participants