Skip to content

fix: sendRequest doesnt return error in some cases - INS-1262 - #9059

Merged
ihexxa merged 3 commits into
developfrom
fix/sendReq
Sep 8, 2025
Merged

fix: sendRequest doesnt return error in some cases - INS-1262#9059
ihexxa merged 3 commits into
developfrom
fix/sendReq

Conversation

@ihexxa

@ihexxa ihexxa commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

Changes

  • Return error when there's error in sendRequest
  • requestToCurlOptions always returns request body mime type as the body could be empty

Tested cases from: #9031 #8897

@ihexxa
ihexxa requested review from ZxBing0066 and xiaodemen August 26, 2025 08:59
@ihexxa ihexxa self-assigned this Aug 26, 2025
Comment thread packages/insomnia-scripting-environment/src/objects/send-request.ts Outdated
@ZxBing0066

Copy link
Copy Markdown
Member

Seems there's some boilerplate code. Could we make the code simpler? like:

const promise = new Promise<Response | undefined>(async (resolve, reject) => {
  try {
    // ...
    resolve(result);
  } catch (e) {
    // ...
    reject(e);
  }
})

if(cb) {
  try {
    const result = await promise;
    cb(undefined, result);
  } catch(e) {
    cb(e);
  }
}

return promise;

ZxBing0066
ZxBing0066 previously approved these changes Aug 27, 2025
@ihexxa

ihexxa commented Aug 27, 2025

Copy link
Copy Markdown
Contributor Author

Seems there's some boilerplate code. Could we make the code simpler? like:

const promise = new Promise<Response | undefined>(async (resolve, reject) => {
  try {
    // ...
    resolve(result);
  } catch (e) {
    // ...
    reject(e);
  }
})

if(cb) {
  try {
    const result = await promise;
    cb(undefined, result);
  } catch(e) {
    cb(e);
  }
}

return promise;

Cool, will refactor it a bit.

@jackkav jackkav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This promise callback is an async function, why not just await the response? why use these promise then chains? Very difficult to understand error cases.

@ihexxa

ihexxa commented Sep 2, 2025

Copy link
Copy Markdown
Contributor Author

@jackkav Firstly I agreed that await the response is more reasonable, there's background of it, we digged into this before, as the existing interface which only supports callback, which doesn't require await while it works in kindof synchronous way, the weird behavior makes the implementation tricky. I will take a look to see if i could make it more straightforward but honestly I'm not sure how much can be improved.

Comment thread packages/insomnia-scripting-environment/src/objects/send-request.ts
@ihexxa
ihexxa merged commit a8d4b60 into develop Sep 8, 2025
14 checks passed
@ihexxa
ihexxa deleted the fix/sendReq branch September 8, 2025 09:56
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