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(templates): ensure requestOptions follow same format #506

Merged
merged 6 commits into from
May 17, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented May 16, 2022

🧭 What and Why

🎟 JIRA Ticket: -

Changes included:

Changes in this PR were added in #501, but are unrelated to the task.

Java/JavaScript:

  • Fix type for queryParameters

Java/JavaScript/PHP:

  • Ensure requestOptions.queryParameters are handled the same way than method queryParameters

PHP:

  • Remove hardcoded case for array query parameters, they should all be treated the same way

🧪 Test

CI :D

@shortcuts shortcuts requested review from damcou and millotp May 16, 2022 15:55
@netlify
Copy link

netlify bot commented May 16, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 8fc5b3d
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6283685b603b8b00082d57ff

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 16, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@shortcuts shortcuts changed the title fix(clients): ensure requestOptions follow same format fix(templates): ensure requestOptions follow same format May 16, 2022
@shortcuts shortcuts force-pushed the fix/serialize-query-parameters branch from 3cc3d52 to 837b478 Compare May 16, 2022 16:05
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Nice ! I'm not entirely sure of when query params should be stringified, should it be last second before creating the URL, or as soon as we receive them they get stored as string ?

@shortcuts
Copy link
Member Author

shortcuts commented May 17, 2022

Nice ! I'm not entirely sure of when query params should be stringified, should it be last second before creating the URL, or as soon as we receive them they get stored as string ?

I've went with what was the easiest for the current implem to avoid changing everything, I feel like to avoid redundancy it's best to apply it when we will use them rather than in every methods

@shortcuts shortcuts requested a review from millotp May 17, 2022 08:49
damcou
damcou previously approved these changes May 17, 2022
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Nice improvement ! I reviewed and tested the changes on my local env and it works fine ! :)

@shortcuts shortcuts requested review from damcou and millotp May 17, 2022 09:38
// handled in the `serializeUrl` step right after.
if (
!value ||
Object.prototype.toString.call(value) === '[object Object]'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how we can get object here

Copy link
Member Author

Choose a reason for hiding this comment

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

const requestOptions = {
  queryParameters: {
    myParam: { a: 'b' },
  },
}

idk if we should allow this in JS but as it's the case in the current client, we can keep the same logic

Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

I'm not sure about all this but let's go

@shortcuts shortcuts merged commit 9320a5f into main May 17, 2022
@shortcuts shortcuts deleted the fix/serialize-query-parameters branch May 17, 2022 09:54
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.

4 participants