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

feat(core,legacy-scripting-runner): allow GET requests to have body #195

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Apr 10, 2020

Adds a hidden option allowGetBody to z.request(). When allowGetBody is true, z.request won't delete the body for the GET request. This is to add support for WB's ability to send GET requests with a body.

This has to be done with a nasty hack because node-fetch explicitly forbids us to send a GET request with a body (see also whatwg/fetch#551). Not an ideal solution, but it gets the job done.

@eliangcs eliangcs marked this pull request as ready for review April 10, 2020 07:13
@eliangcs eliangcs requested a review from xavdid as a code owner April 10, 2020 07:13
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

woof, this one is gross. The only improvement i could think to make is restricting this to apps w/ the legacy property so that there aren't any proper CLI apps that take advantage of this. Not a blocker if that's not trivial to add though.

Thanks!

@eliangcs
Copy link
Member Author

eliangcs commented Apr 13, 2020

@xavdid I think this "feature" can still be useful for CLI apps. We probably don't want to encourage this usage—that's why I made it a "hidden" option. But there are APIs like ElasticSearch that requires users to GET with body. So we should at least provide an option for developers to do it.

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.

2 participants