Add an Option to allow non-zero exit codes when failures happen

[goldsziggy]

Is your feature request related to a problem? Please describe. I would like to script the generation of the swagger document, however because errors are only logged at the moment I have no clear way of failing the script when the swagger wasn't correctly parsed (outside of piping then greping the output). If we have an option to provide non-zero exit codes when this fails the script will naturally fail solving this issue.

Describe the solution you'd like Add an option to the file specification.js to allow for non-zero exit codes. something like failOnErrors. We can default the boolean to false to retain existing behaviors.

Describe alternatives you've considered Manually pipe and grep the output of the command checking for the error strings.

[kalinchernev]

Hi @goldsziggy I don't mind having a new option for the behavior you're suggesting.

As a background info, initially, behavior was throwing as usual, but then the question of displaying errors was getting way bigger than necessary, such as formatting pointers in the terminal for showing fancy ways of what exactly is wrong. And because I want to have tests for all features and it was simply shitty to maintain, and together with changing the yaml parser for handling references: I changed the behavior.

What we can do as option one is having a similar approach as the encoding, having it as an optional parameter (used later almost directly), could be added here maybe https://github.com/Surnet/swagger-jsdoc/blob/master/src/specification.js#L262

    if (errReport.length) {
      if (options.failOnErrors) {
        throw new Error(errReport)
      }
      console.info(
        'Not all input has been taken into account at your final specification.'
      );

      console.error(`Here's the report: \n\n\n ${errReport}`);
    }

If you prefer, we could also make a callback approch, so that we have something like options.onParseFailure and

swaggerJsdoc({
  onParseFailure: (report) => {
    // Send the report to another service?
    throw new Error(errReport);
  } 
})

    if (errReport.length) {
      if (options.onParseFailure) {
        options.onParseFailure(errReport)
      }
      console.info(
        'Not all input has been taken into account at your final specification.'
      );

      console.error(`Here's the report: \n\n\n ${errReport}`);
    }

[goldsziggy]

Yeah Option 1 seems to be a better use-case for this, as it feeds quickly into the use-case of just fail-fast. If others have use-cases later on I am super open to the callback approach, but I think it just may be a bit heavy handed if the most common use-case is either report only or fail.

As a quick aside as well -- I'm wrapping up my blog post on how I am using your repo. This last bit is so I can write a section on using it in CI and pre-commit hooks to ensure you block updates that break the swagger speciffication.

https://gem-ini.medium.com/de-duping-the-duplication-in-services-featuring-swagger-openapi-and-ajv-abd22c8c764e

[kalinchernev]

Hey @goldsziggy I squeezed some time before the working day today to try to provide the feature as the discussed option 1 in 6.1.0 I think this commit says a thousand words https://github.com/Surnet/swagger-jsdoc/commit/e85a78d836105ffc24a9db773a44639f8a544186

Let me know if it's not quite exactly what you need?

I'm very happy to have you in the loop of the project!

I like the article too! I'll be glad to have it in this section as an inspiration for others https://github.com/Surnet/swagger-jsdoc/blob/master/docs/FIRST-STEPS.md#further-resources

[goldsziggy]

Awesome, thanks for taking care of that, I was going to get you an MR for it later today too.

Also great to hear about that section, I'll throw it there in send you a PR if that works!

[kalinchernev]

Please do make a pull request for adding the article, yes :)

[kalinchernev]

Hey @goldsziggy thanks for submitting the article and writing it! :) I noticed that in the last section the swagger-generator.mjs is ES6 + 7.x whereas the rest of the code is CJS. Which version you're using? I still hold 7.x in RC as i don't have full confidence in the ESM rewrite, but I already feel a bit the overhead of having 6 and 7 in parallel and i'll be happy to get your feedback about the stability of 7.x. If you use it primarily and it's not so bad maybe i can push it as a main? Asking because I really appreciate your feedback in the last we weeks and 6.x could have been more stable before going into main if i had had your feedback before :)

[goldsziggy]

So to be honest I've only used the 6.* version. I ended up making my script as a mjs to help future proof it as I saw version 7 was ES6.

That script will work with both version 6 and 7.

As for the v7 release, I think if it goes stable it'll be a huge breaking change for a lot of repos. Have you tried using something like rollup (or your bundler of choice) to take the v7 release and add an optional CJS release?

After you build it for both styles you can then specify how each load in your package.json.

I can throw a PR to try something like that if you'd like

[kalinchernev]

I see, good idea to have mjs to cover both styles. However, pls keep in mind that 6.x is not returning a promise as provided in the example.

About the potential breaking change, yes, that's my biggest fear. Spent weeks refactoring the repo for the third time in the last year and I had to make compromises which i still don't like.

I've thought many times on the subject of bundlers and i'm still not sure whether it's worth adding one in the toolchain. I mean, I've been always trying to keep it as native as node.js can go, but with this ESM migration for the first time it felt wrong to so much for visibly simple things.

The main issue with bundlers in my point of view is their nature of surely being temporary solution. I know enough rollup or webpack, but what if Vite gains traction? :D

As an idea in the back of my mind is typescript. To my knowledge 1 compiler will do for all transpilations necessary and it's integrated well with other tools.

Another thing which doesn't give me peace is the fs reading/writing part which i refactored to be promise based in 7.x which introduced the new promise-based nature which turned out to be of no real gain, fs sync functions turned out faster than async code which doesn't pay off enough...

I'll think about this a bit more, but you're right that a purely ESM + new async signature will be hard to reason instead of adding a transpilation step and keeping all latest in 6

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JoseGuillermoAraya]

The documentation for this option is missing I think.