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 api-request leaking state when changing paths. #164

Conversation

saraid
Copy link
Contributor

@saraid saraid commented Jul 14, 2023

Reproduction:

  1. Given two path items with mutually exclusive sets of mimeTypes,
{ paths: {
  "/foo": {
    "post": {
      "requestBody": {
        "content": {
          "application/octet-stream": {}
        }
      }
    },
    "put": {
      "requestBody": {
        "content": {
          "application/json": {}
        }
      }
    }
  }
}}
  1. Navigate between them. You'll see errors in the JS console. The UI has no problems.

The error indicates that the reqBody is undefined, because it was searching for the incorrect selectedRequestBodyType in requestBodyTypes. This fix is just patching it so that, if it fails to find the "selected" requestBodyType, it simply picks one that at least exists.

Reproduction:
1. Given two path items with mutually exclusive sets of mimeTypes,
```js
{ paths: {
  "/foo": {
    "post": {
      "requestBody": {
        "content": {
          "application/octet-stream": {}
        }
      }
    },
    "put": {
      "requestBody": {
        "content": {
          "application/json": {}
        }
      }
    }
  }
}}
```
2. Navigate between them. You'll see errors in the JS console. The UI
   has no problems.

The error indicates that the `reqBody` is undefined, because it was
searching for the incorrect `selectedRequestBodyType` in
`requestBodyTypes`. This fix is just patching it so that, if it fails to
find the "selected" requestBodyType, it simply picks one that at least
exists.
@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@saraid
Copy link
Contributor Author

saraid commented Jul 14, 2023

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Jul 14, 2023
@@ -411,7 +411,8 @@ export default class ApiRequest extends LitElement {
`;

// For Loop - Main
const reqBody = requestBodyTypes.find(req => req.mimeType === this.selectedRequestBodyType);
const reqBody = requestBodyTypes.find(req => req.mimeType === this.selectedRequestBodyType) || requestBodyTypes[0];
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally sure this is the right fix, the solution seems a bit strange. According to the problem, my guess would be we need to reset the selectedRequestBodyType on route change by watching the input parameters. Or more specifically we could watch this.request_body changes and the update the selected value.

Copy link
Member

@wparad wparad left a comment

Choose a reason for hiding this comment

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

Thank you for finding the problem and attempting a fix here, we really appreciate it. I will say that I believe there might be a better way to handle this. As it is right now, I'm also worried about an infinite loop because we are changing the value of a property that is the one that triggered the function in the first place. I've made a suggestion inline, let me know if you aren't sure how to implement that, and I can go over an example implementation.

@wparad wparad merged commit 250a94c into Authress-Engineering:release/1.0 Jul 21, 2023
3 checks passed
@wparad
Copy link
Member

wparad commented Jul 21, 2023

Great thank you, this has been merged in #167 to the release 1.0. If you wait a few hours a version should be released!

wparad pushed a commit that referenced this pull request Sep 20, 2023
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