Swagger docs #892
Swagger docs #892
Conversation
Mmh yes indeed, considering that it will also lint the swagger definitions. Let's see what @Hypfer thinks |
|
Are there any downsides? |
Swagger compilation takes < 1s, I would add it to the build command (and therefore to CI jobs). This also implies that it is validated on every build: I pulled in a swagger validator that is run after generating |
|
Ready for review as soon as the build stops failing. Still left to do (not by me :) ):
Btw, the swagger endpoint is |
|
I do have to say that writing YAML inside a jsdoc-style comment is insanely painful and I have absolutely no idea how you've managed to document 80% of the whole application without throwing your computer out of the window. Can we do something so that this becomes less of a pain? What I also strongly dislike is that apparently there's no way to reuse the existing types in the swagger docs which leads to enums being duplicated for no good reason. |
|
So i just found this error by actively looking at the generated swagger.json and not because anything failed or warned me or was visible via syntax highliting anywhere else
And as it turns out, the indentation is wrong. Also, what is this object schema view? Why is everything spaced half a screen apart Why do we use jsdoc when it doesn't even care about the already existing and defined types? I've been spending the last hour to research if we're doing something wrong here. How is that even possible? How can anything this developer-hostile the best thing currently available in the market? While I really appreciate your effort here, I'm afraid that I can't merge this or anything that adds swagger support, since I can guarantee that I will loose any motivation to maintain this project the moment 1/3 of every file becomes comment YAML and every time I look at the API I'm greeted with "Swagger by $BRAND_NAME".
It seems to me that OpenAPI prioritizes the API-user experience over the API-developer experience, which of course makes sense if you're trying to sell a product. With Valetudo however, there's no such incentive and therefore I'd strongly prefer developer experience over anything else. Personally, I'd instantly walk away from a project when confronted with that yaml hell and a growing list of docs and other things that you'll have to update manually when changing basically anything about the project. I don't want the Valetudo PR template to become a list of checkboxes that one has to read and check every single time just because the tooling is so bad that things need constant reminding instead of just working as intended. Anyways, sorry for the rant and your wasted time. |
I'd love to tell you there's a better option but no. This is the best you can get with JavaScript at the current state of the art (assuming we can call it "art" cause I don't think it's very appropriate). This is the same rant I've always had at work and this is the reason why I'd write a web application in C rather than using JavaScript. If that's the environment I have to work in, I'd rather work on memory corruption bugs. When I had used it at work I mostly worked on Java and with Java everything works waaaay more smoothly, basically the Swagger implementation can infer pretty much everything that's required from the source code.
With TS you can do it to some extent, but with that being said (and also mentioning that the guys at the company are actually very good and really against this type of duplication) we'd still add the yaml definition on top, even on TS. And considering that TS is imo absolutely not worth the pain to begin with, 🤷♂️ On top of that, Express has an horrendous API and there's basically nothing you can do to automatically infer the request structure etc, as opposed to stuff like SpringBoot or even just Flask.
That triggers me as well and I was going to add some CSS to remove it 🤣 Now, a few counterarguments:
I think there is an extension for VSCode. I found nothing for WebStorm but you get the hang of it, "space space" instead of pressing tabs and Alt+J for changing indentation for multiple lines. Also, for the most part you can just copy-paste from other endpoints, most of them have the same "layout". Not ideal or particularly "fun", but also not as bad as you say.
Mmh I think you're overwhelmed by the size of the PR. I don't think it's particularly unmanageable. It's pretty bad, but it could have been much much worse. Overall, what has to be documented with the
While I'd love the second to not be the case, there's not much I can do. But on the other hand, the standard used for those is JSONschema, which isn't (imo) bad at all. It is a separate standard from Swagger, fyi. For the routers, the REST APIs really need some sort of formalization (see again my comment on Express) and I think that this type of comment does the job just fine. All the time someone will spend adding the definition for their new or updated REST endpoint is time well spent and it is 1/4 the time some person wanting to use the REST API will spend finding the endpoint's source code and jumping around it to find all the allowed payloads and evaluating all the ways it works.
I don't think having separate files is a good idea since new stuff will never be documented. Having the docs close to the code they document adds to the guilt of doing things halfway.
Are you sure? The code under ConclusionsI totally understand your concerns and your outrage was exactly the same I felt when I first used Swagger with TS at work. However, after letting the outrage boil down and after accepting that if you work with JS and Express this is what you have to deal with, I think that the overall advantages are more than the disadvantages. The disadvantages also do not make the code unmaintainable. It's just that they could have thought it a lot more thoroughly. I think this for many other JS libraries that I hate: Express is one of them, another one is Jest. I'm not going to go into exactly why but while I think Express is one of the worst libraries ever written - only Jest is worse than it - we can still agree that having shared code for managing bug-prone HTTP stuff and that having tests is better than not. Finally, here's some quotes from screenshots I have from when I used to work with TS, translated and censored:
and from my shell config
tl;dr: You used JavaScript, what did you expect? :P When you decide to port the backend to golang or rust, hit me up :P |
|
I didn't tried it yet, but what about using express-jsdoc-swagger? |
|
Thank you for taking the time to respond to that!
Yes please So there are a few reasons for why I don't like the comments as they are now:
Therefore, I've decided to rework that and instead add a This way, they are still right next to the files that they should document while not bloating them and requiring us to strip them from the binary in the build process. Also, I can easily exclude Furthermore, it's not YAML. That alone should be more than enough reason to do it Funfact: This needs the empty line in YAML because there's no line-break otherwise. Two is one and one is none. \n gets ignored. |
|
https://brikev.github.io/express-jsdoc-swagger-docs/#/requestBody Hmm. That actually looks like it could re-use the existing stuff which would be even better |
I like this one, thanks for the suggestion :)
That's also a valid compromise, I'm not a huge fan but it works. Let's make it still YAML though, JSON would be unreadable.
That's still a good improvement imo. Something like this: |
* Import/convert from #892 * docs(swagger): Add swagger definitions for SystemRouter * Swagger is an optional feature and should be treated as such * Remove $brand * docs(swagger): Add swagger definitions for NTPClientRouter * chore: Fix package-lock * chore: Fix package-lock * docs(swagger): Document State Attributes and Map Layer + Map Entities * Use tagged version * fix pkg build * add schema validation * remove unnecessary custom jsdoc tag Co-authored-by: Davide Depau <davide@depau.eu>
Type of change
Type A:
This PR introduces Swagger API docs.
Docs are generated at build-time and added to
client/swagger.json, which I gitignored and should be force-added when needed.An alternative to adding it to the client could be to add them to the docs website. However we still need to add the swagger "client" libraries to the UI because CORS etc.