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

feat(javascript): use the text/plain content-type #614

Merged
merged 1 commit into from
May 31, 2022

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented May 31, 2022

🧭 What and Why

in algolia/react-instantsearch#3487 I discovered the text/pain content-type is also accepted by the engine, and also doesn't create a full CORS request.

text/plain is preferable over application/x-www-form-urlencoded as the request we send isn't actually form encoded which means that debug tools like the network panel won't parse them wrong.

🎟 JIRA Ticket:

Changes included:

  • content-type for JS client changed to text/plain

🧪 Test

run requests and see that everything works with text/plain, especially including different clients like recommend.

in algolia/react-instantsearch#3487 I discovered the text/pain content-type is also accepted by the engine, and also doesn't create a full CORS request
@netlify
Copy link

netlify bot commented May 31, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 185d145
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6295e33c75c39d000898a0e8

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 31, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

@@ -91,7 +91,7 @@ export function create{{capitalizedApiName}}(options: CreateClientOptions{{#hasR
requestsCache: options.requestsCache,
responsesCache: options.responsesCache,
baseHeaders: {
'content-type': 'application/x-www-form-urlencoded',
'content-type': 'text/plain',
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably be enforced by a unit test (not sure where and how)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous version didn't seem tested either. An extra test per method checking it sends the right content type maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think CTS checks the headers but never really took a look at it. Pierre will have the answer ☺️

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can test for headers but we don't actually use it yet, because it was the same everywhere, definitely the time to test it ! For example in tests/CTS/methods/requests/search/assignUserId.json we check the value of headers.x-algolia-user-id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is only done in the JS client (other clients always use application/json), and itself mostly for the browser (node could use application/json too), I couldn't find a good place for those tests

Haroenv added a commit that referenced this pull request May 31, 2022
In v4 these are all allowed, and this allows users to work around possible issues we have smoothly, see eg. algolia/react-instantsearch#3487

At the same time I also simplified the passing of options in node/browser, they were double checking a variable that later overrides the original with the spread anyway.

Note to self: #614 touches a similar part of the code, whichever merges last will have to deal with a conflict
@Haroenv Haroenv requested a review from bodinsamuel May 31, 2022 13:43
@millotp millotp merged commit 8995c36 into main May 31, 2022
@millotp millotp deleted the feat/content-type-text branch May 31, 2022 13: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.

None yet

4 participants