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

Fix raw input (JSON) #693

Merged
merged 9 commits into from
Mar 22, 2020
Merged

Fix raw input (JSON) #693

merged 9 commits into from
Mar 22, 2020

Conversation

leomp12
Copy link
Contributor

@leomp12 leomp12 commented Mar 20, 2020

Related issue: #597

As I've commented on commit b14adc2, the problem with JSON body and raw input is happening again after this commit, maybe I've named things badly (canListParameters) and you misunderstood me, so I'm suggesting another fix 😛

I've realized that it's getting rawInput original state from Vuex, and then it could happen to have JSON content with raw input disabled and without the toggle component, this way it would not be possible to enable raw input at all...

Now I'm making sure rawInput is true always canListParameters is false, and kept (revert) canListParameters to be true only with application/x-www-form-urlencoded.

With immediate canListParameters wacther, https://github.com/liyasthomas/postwoman/compare/master...leomp12:fix/raw-input?expand=1#diff-b19fa22add3de9cd0b98b05366479d92R1767 may be unnecessary, just to be really sure

@ghost
Copy link

ghost commented Mar 20, 2020

Congratulations 🍻. DeepCode analyzed your code in 0.512 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 8ee45720-6adc-11ea-8310-d706d82aa088

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: de411ce0-6adc-11ea-8310-d706d82aa088

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 31e8f9d0-6add-11ea-8310-d706d82aa088

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 20, 2020

@liyasthomas lot of bad commits here, so if you agree with changes I would suggest to squash the PR 😅

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 4fb5cd20-6ade-11ea-8310-d706d82aa088

@liyasthomas
Copy link
Member

liyasthomas commented Mar 20, 2020

@leomp12 PR looks great! But I've been getting feedbacks from users suggesting I should enable RAW input for content-types otherthan application/x-www-form-urlencoded. For example: application/json should also support RAW payload, if not.. user need to enter payload params one-by-one. Which sucks.

Hope you can understand why I hotfixed that yesterday. Please add support for Raw input for all content-types expect text/html and application/xml

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 21, 2020

Hey @liyasthomas , not sure if I undestand you 🤔

This PR is exactly ensuring JSON content will be with RAW payload, I'm not suggesting remove raw input for JSON, is the opposite.

@leomp12 leomp12 closed this Mar 21, 2020
@leomp12 leomp12 reopened this Mar 21, 2020
@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: ecc56340-6b0c-11ea-95ea-63bd0204df09

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 21, 2020

But I've been getting feedbacks from users suggesting I should enable RAW input for content-types otherthan application/x-www-form-urlencoded

It's exactly what I'm doing here, forcing raw input for all content types other than application/x-www-form-urlencoded, then only form encoded will have raw input toggle to be able to enable/disable, in other cases raw input is always true (enabled) and with no option to disable.

I mean, with this PR users would not be able to disable RAW input for JSON, do you disagree with that?

@liyasthomas
Copy link
Member

liyasthomas commented Mar 21, 2020

@leomp12

This PR is exactly ensuring JSON content will be with RAW payload, I'm not suggesting remove raw input for JSON, is the opposite.

I understand this PR exactly does that, but at the same time it also remove the toggler to disable raw input right? What I recommend is that, we should allow user to have the freedom to toggle between Raw input and key-value input (not jut raw input) for all content-type except text/plain and application/xml.

sorry if there was any confusion.

It's exactly what I'm doing here, forcing raw input for all content types other than application/x-www-form-urlencoded

Don't do that. Allow both Raw input and key-value pair.

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 21, 2020

Ah ok, now I understand you 😃
Sorry for that, I think JSON was not working with parameters list at all (don't work on my tests I think).
So maybe we can just set raw input enabled by default for JSON, but with the toggle, wdyt?

@liyasthomas
Copy link
Member

So maybe we can just set raw input enabled by default for JSON, but with the toggle, wdyt?

Perfect!

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 21, 2020

Great, I'll commit a fix tomorrow

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: c6ec81a0-6be6-11ea-a42a-ffb46b271005

@leomp12
Copy link
Contributor Author

leomp12 commented Mar 22, 2020

Hello again @liyasthomas ,
Now it's showing the raw input toggle for both JSON and form encoded and it'll be raw by default only when rawParams is edited.
So even for JSON I'm keeping raw disabled by default (as it was), but when user enable raw input it'll be preserved through URL query, sounds good?

@TravisBuddy
Copy link

Hey @leomp12,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: b84dd6d0-6bf5-11ea-a42a-ffb46b271005

@liyasthomas liyasthomas merged commit e7e960e into hoppscotch:master Mar 22, 2020
@leomp12 leomp12 deleted the fix/raw-input branch March 22, 2020 04:35
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.

3 participants