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

Add Swagger UI endpoint #1144

Merged
merged 5 commits into from
Feb 10, 2025
Merged

Add Swagger UI endpoint #1144

merged 5 commits into from
Feb 10, 2025

Conversation

perfectra1n
Copy link
Collaborator

@perfectra1n perfectra1n commented Feb 9, 2025

Currently it's just /api-docs...but I'm open to changing it since that's not the "best looking" endpoint lol

image

Got the idea from #1123.

@eliandoran
Copy link
Contributor

eliandoran commented Feb 9, 2025

What about on GET /etapi/?

@pano9000
Copy link
Contributor

pano9000 commented Feb 9, 2025

General observations:

  • that seems to be bringing in quite a big dependecy: 11 MB for https://www.npmjs.com/package/swagger-ui-dist for displaying swagger
  • the swagger-ui-express would be able to load .json file directly, without an extra js-yaml dependency:
    • maybe it's worth it to convert the yaml file to JSON? Not sure, just a thought
  • I would avoid using readFileSync and instead use the async version via fs/promises

edit:
alternative idea would be to link to https://editor.swagger.io/, but that would of course come with some downsides as well (i.e. you would need to rely on a external service again).

@perfectra1n
Copy link
Collaborator Author

perfectra1n commented Feb 9, 2025

that seems to be bringing in quite a big dependecy: 11 MB for https://www.npmjs.com/package/swagger-ui-dist for displaying swagger

Yeah, the Swagger UI will increase the packages size but does offer quite a bit of functionality as a tradeoff.

the swagger-ui-express would be able to load .json file directly, without an extra js-yaml dependency: maybe it's worth it to convert the yaml file to JSON? Not sure, just a thought

I'm open to converting the YAML OpenAPI file to JSON, but I have found it more commonly stored as a YAML format. Maybe there's another package I can use to load it without adding another dep.

I would avoid using readFileSync and instead use the async version via fs/promises

Sure, I can change that!

@pano9000
Copy link
Contributor

pano9000 commented Feb 9, 2025

I'm open to converting the YAML OpenAPI file to JSON, but I have found it more commonly stored as a YAML format. Maybe there's another package I can use to load it without adding another dep.

don't worry about it, it probably isn't worth investigating it, it was just something that popped up, while reading it.
I agree, that in 95% of the cases, you will find the file in yaml format, and it does look a tiny bit weird looking at the converted file in JSON format :-D

@perfectra1n
Copy link
Collaborator Author

Yeah, we would need the js-yaml package to load the YAML into Swagger UI. I've gone ahead and changed the Site Title as well to match what Elian suggested.

const __dirname = dirname(fileURLToPath(import.meta.url));
const swaggerDocument = yaml.load(
await readFile(join(__dirname, "../etapi/etapi.openapi.yaml"), "utf8")
) as object;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit of a nitpicking comment:

the swaggerUI types seem to suggest that it takes a JsonObject type
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/84e6da3397915d415adf7ef5b636affd8638b447/types/swagger-ui-express/index.d.ts#L38

the js-yaml types suggest that "load" returns an unknown type
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/84e6da3397915d415adf7ef5b636affd8638b447/types/js-yaml/index.d.ts#L3

So I think if we do a typecast here, it should be OK to typecast this to "JsonObject" instead.

what do you think?

(you might need to use import type to import the definition)

@pano9000
Copy link
Contributor

that last commit was more about making the TypeScript gods not flinch, when they see that typecasting ;-)

it looks good for me now, thank you (and sorry for nitpicking ;-))
I will leave it up to @eliandoran to decide if he want to also include Swagger UI built in or not :-)

@pano9000 pano9000 removed the question label Feb 10, 2025
Copy link
Contributor

@eliandoran eliandoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for now, however in the future I would hope to generate the documentation statically and serve that instead if it reduces the binary size.

@eliandoran eliandoran merged commit d0399c1 into develop Feb 10, 2025
5 checks passed
@eliandoran eliandoran deleted the feature/swagger-ui-in-server branch February 10, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants