elements icon indicating copy to clipboard operation
elements copied to clipboard

fix(elements-core): Do not omit request body for certain HTTP methods

Open provokateurin opened this issue 1 year ago • 2 comments

  • [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.

provokateurin avatar Jul 01 '24 08:07 provokateurin

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jul 01 '24 08:07 netlify[bot]

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jul 01 '24 08:07 netlify[bot]

@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.

mnaumanali94 avatar Jul 12 '24 11:07 mnaumanali94

Done!

provokateurin avatar Jul 12 '24 12:07 provokateurin

Hey @mnaumanali94 do you mind taking another look? Would be nice to have this fix available soon :)

provokateurin avatar Jul 22 '24 13:07 provokateurin

Is the e2e failure related? I can't tell from the logs it's giving

provokateurin avatar Jul 24 '24 07:07 provokateurin

@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).

darekplawecki avatar Jul 29 '24 09:07 darekplawecki

Ok I can bump the version. Should I wait for you to check the tests or do it right away?

provokateurin avatar Jul 29 '24 09:07 provokateurin

@provokateurin no need to wait, you can do it right away :)

darekplawecki avatar Jul 29 '24 11:07 darekplawecki

@provokateurin I checked E2E Tests. They are failing while trying to send a request:

Screenshot 2024-07-29 at 14 58 03

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.

darekplawecki avatar Jul 30 '24 08:07 darekplawecki

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 avatar Jul 31 '24 09:07 provokateurin

@provokateurin sounds good to me :)

darekplawecki avatar Jul 31 '24 11:07 darekplawecki

@provokateurin If you want to take out the stuff around try it, we can review this again.

mnaumanali94 avatar Aug 02 '24 11:08 mnaumanali94

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.

provokateurin avatar Aug 02 '24 11:08 provokateurin