fix(elements-core): Do not omit request body for certain HTTP methods
- [x] Read
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.
Deploy Preview for stoplight-elements ready!
| Name | Link |
|---|---|
| Latest commit | 3b8b13cef4ac860e8d57af317f099ee9a9391ca7 |
| Latest deploy log | https://app.netlify.com/sites/stoplight-elements/deploys/66a78f846d3e0f00084e827e |
| Deploy Preview | https://deploy-preview-2607--stoplight-elements.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Deploy Preview for stoplight-elements-demo ready!
| Name | Link |
|---|---|
| Latest commit | 3b8b13cef4ac860e8d57af317f099ee9a9391ca7 |
| Latest deploy log | https://app.netlify.com/sites/stoplight-elements-demo/deploys/66a78f8493b2640008006a6e |
| Deploy Preview | https://deploy-preview-2607--stoplight-elements-demo.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@provokateurin Tests seem to be failing particularly the lint and check step. Others probably are unrelated. Can you take a look and fix them please. Then you can ask for a re review.
Done!
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 package.json. Since you've made changes in elements-core, you would need to upgrade version in all three packages (read Versioning Guideline).
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 :)
@provokateurin I checked E2E Tests. They are failing while trying to send a request:
This is the error that is thrown:
Failed to execute 'fetch' on 'Window': Request with GET/HEAD method cannot have body.
So it seems like the fetch method from window API in browser forbids request body for GET requests and it breaks the "Try It" functionality.
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 :/ Maybe parts of this PR can still be merged, since e.g. curl probably allows this behavior and there is no reason to prevent it. For the TryIt a warning could be displayed that explains why the body isn't visible/in the request.
@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.