-
Notifications
You must be signed in to change notification settings - Fork 196
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
fix(elements-core): Do not omit request body for certain HTTP methods #2607
fix(elements-core): Do not omit request body for certain HTTP methods #2607
Conversation
✅ Deploy Preview for stoplight-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for stoplight-elements-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@provokateurin Tests seem to be failing particularly the |
d20e02d
to
ff22efd
Compare
Done! |
ff22efd
to
0e36b14
Compare
Hey @mnaumanali94 do you mind taking another look? Would be nice to have this fix available soon :) |
Is the e2e failure related? I can't tell from the logs it's giving |
@provokateurin The code looks fine, but if you want to release changes, you also need to upgrade version in I'll check E2E Tests however they are in general flaky and therefore not really reliable (we have a ticket in our sprint to analyse it). |
Ok I can bump the version. Should I wait for you to check the tests or do it right away? |
@provokateurin no need to wait, you can do it right away :) |
de037dc
to
231e8af
Compare
231e8af
to
3b8b13c
Compare
@provokateurin I checked E2E Tests. They are failing while trying to send a request: This is the error that is thrown: So it seems like the |
Even though I think it is wrong, Chrome seems to prevent this. I already saw this in a real codebase where a GET request had a body and failed, so I don't think there is a way around this :/ |
@provokateurin sounds good to me :) |
@provokateurin If you want to take out the stuff around try it, we can review this again. |
Sorry, I'm dealing with other problems at the moment. I'm not sure I will revisit this soon, so feel free to close this PR or pick up the working parts of it. |
CONTRIBUTING.md
The HTTP spec does not forbid using request bodies for GET, DELETE and other methods (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/GET for example). OpenAPI doesn't forbid this either, so it is possible to have a valid specification that is not correctly executed by this library.
I'm not sure about adding tests, as I couldn't find any tests that cover this area of the code so far, or at least not directly unit testing this method. Let me know if you want to have tests for this change and how I can implement them best.