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: adds content type only on post method #198

Merged

Conversation

mohsen-khalili
Copy link
Contributor

In sdk for set GET request adds content type in header but not necessary and common waf unit block this request, i change code and set content type header adds only in POST request.

[Closes #197 ]

Copy link
Contributor

@thomasheartman thomasheartman left a 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! It makes sense to not send this header if there is no body, so I appreciate the effort. However, I'd like to see this tested before we get it in.

In src/index.test.ts we have two tests that inspect the requests for both get and post cases:
'Should perform an initial fetch as POST' and 'Should perform an initial fetch as GET'.
You should be able to inspect the headers of the request here and ensure that the header is present on post requests and that it is not present on get requests.

Does that sound alright? 😄

src/index.ts Outdated Show resolved Hide resolved
@mohsen-khalili mohsen-khalili force-pushed the fix/remove-content-type-on-post branch from bfa3f3c to 08c8753 Compare March 13, 2024 06:35
@mohsen-khalili
Copy link
Contributor Author

Thanks for review
I fixed test for check header in GET and POST request please check it
@thomasheartman

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice work on this one 🔥 I'll commit the suggestion I put in for the test and get this merged right away. Thanks for the contribution 🙏🏼

src/index.test.ts Outdated Show resolved Hide resolved
@thomasheartman thomasheartman enabled auto-merge (squash) March 13, 2024 09:29
@thomasheartman
Copy link
Contributor

Oh, @mohsen-khalili, one more thing: CI is failing because the formatting is inconsistent with prettier. Can you make sure you run prettier on src/index.ts, please?

yarn run v1.22.22
$ prettier src --check && eslint src
Checking formatting...
[warn] src/index.ts
[warn] Code style issues found in the above file. Run Prettier to fix.

auto-merge was automatically disabled March 16, 2024 10:43

Head branch was pushed to by a user without write access

@mohsen-khalili mohsen-khalili force-pushed the fix/remove-content-type-on-post branch from 07a94ad to aa6fb8c Compare March 16, 2024 10:43
@mohsen-khalili mohsen-khalili force-pushed the fix/remove-content-type-on-post branch from aa6fb8c to 03a3934 Compare March 16, 2024 10:54
@mohsen-khalili
Copy link
Contributor Author

I fixed it
Sorry, it took a while

@thomasheartman
Copy link
Contributor

No worries at all! I'll merge once the build succeeds. Thanks again ☺️

@thomasheartman thomasheartman merged commit 5875dbe into Unleash:main Mar 18, 2024
3 checks passed
@chriswk
Copy link

chriswk commented Mar 18, 2024

Released in v3.3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Unleash-proxy-client-js: Conditionally add content-type only on post requests
3 participants