-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fixed Swagger GUI page #102
base: master
Are you sure you want to change the base?
Conversation
@@ -1,6 +1,12 @@ | |||
import { ApiProperty } from "@nestjs/swagger"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use single quotes to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's just how my IDE autocomplete works, should look into how to change this to avoid manual fixing each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
bd57951
to
c1dbc25
Compare
this apparently breaks the tests pretty badly |
I guess something else has gone wrong here, because I ran the tests locally on this branch, and it is just fine. |
What tests? The failure is from e2e test suite, not from the unit tests. |
f6a6d40
to
c536576
Compare
Ah, got it. OK, I'll try to figure out how to fix this. Unfortunately, for now I have no idea why does not my current code work. The |
Can you please help me with this thing? I'm quite new to Swagger and testing in Node. |
Did you try reverting that change I noted? It feels like the only thing you changed that can have runtime effect. |
there are actually more changes like that, basically all the removed parameters from |
if that wont help, i cant really help. i dont do webapps anymore, i don't use nest, and i never used swagger either. also i don't consider this as something very important, this is an example app, its purpose is to show how to work with MikroORM and nest, not how to setup nest with swagger - that part is not that important. i would love to have it there, but its not a requirement really. |
I'm just trying to learn using this app, since it's really short and simple. I found one more place to fix in
But when I use Swagger UI I can't get such response at all, no matter what the input is: I get the HTTP 400 Bad Request instead with informative description. I think what is happening here is really that newman is sending requests NOT to the localhost (the fact that I cannot hit any debug breakpoint kind of confirms this version). I guess this script is not testing the local project, it is testing something else instead. |
I see your point of "just revert it back if it works", but I really don't like the nested approach of As for "why use the @ApiBody() annotation if it should work without it, when we use classes and not interfaces" - for some reason that just doesn't work for me. |
Sounds like you are doing something wrong, the e2e test suite fires queries to the local server. I can literally see the query logs on the app instance when I run the e2e test suite. Should be just about |
Yes, that's exactly what I'm doing. Still no breakpoints are triggered when I run |
Then what is the purpose of https://conduit.productionready.io/api URL? |
Where did you find it? I'd guess it's a default, and the https://github.com/mikro-orm/nestjs-realworld-example-app/blob/master/package.json#L16 |
No, it does not work that way. Direct command line argument has higher priority than the env var with the same name. |
Again, the readme file in e2e folder says
So in fact I was passing |
It seems it's finally working now. The problem yesterday was I that forgot to add |
Well, it does work that way on my end, as well as in the CI 🤷
The env var is called |
@@ -49,7 +49,7 @@ | |||
], | |||
"body": { | |||
"mode": "raw", | |||
"raw": "{\"user\":{\"email\":\"{{EMAIL}}\", \"password\":\"{{PASSWORD}}\", \"username\":\"{{USERNAME}}\"}}" | |||
"raw": "{\"email\":\"{{EMAIL}}\", \"password\":\"{{PASSWORD}}\", \"username\":\"{{USERNAME}}\"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert those changes, this needs to stay the same, its official specs of the https://github.com/gothinkster/realworld project (https://github.com/gothinkster/realworld/blob/main/api/Conduit.postman_collection.json)
Made POST body schemas actually work