-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
[web] fix query editing #5574
[web] fix query editing #5574
Conversation
6c80fde
to
d0863fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and sorry for getting at this so late. :) Please see my question below. 😃
@@ -32,7 +32,13 @@ export default function HttpMessage({flow, message}: HttpMessageProps) { | |||
} else { | |||
url = MessageUtils.getContentURL(flow, message, contentView, maxLines + 1); | |||
} | |||
const content = useContent(url, message.contentHash); | |||
let content = useContent(url, message.contentHash); | |||
if (flow.request.method === "GET" && edit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just from the diff it looks like this will break response editing, or am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I will try to find a more proper way to fix this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and rebased.
59eaa93
to
485ea2f
Compare
Thanks. How come you updated this PR from using JS APIs to adding a new REST API endpoint for queries? |
Do you mean where did I get this idea from, or how did I update this PR? |
If I remember correctly, the initial version in this PR used JavaScript to extract the query from the path (I can't take a look at the code again because you force-pushed). Now there is a new REST endpoint instead. What motivated that change? |
Before using JavaScript to extract the query from the path, I tried to make the After you pointed out that my changes would break response editing, I found that the query replacement is also broken, so I plan to fix it as well. This requires changing the |
485ea2f
to
510c868
Compare
Description
As #4565 (comment) said, an empty body would show when editing a query string. This PR tries to fix the query editing logic. Also, fixed query replacement.
I added a new API for retrieving and updating query string
fix #4565
Checklist