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

backport: custom params serializer support for v0.x #6263

Merged
merged 4 commits into from Mar 28, 2024

Conversation

briwa
Copy link

@briwa briwa commented Feb 27, 2024

Resolves #6262

This will add similar support to #5113 (paramsSerializer: { serializer? } or paramsSerializer(params) { .. }) which was missing in v0.x.

Was hoping if this could be released as a patch for v0.28.x

@briwa briwa marked this pull request as ready for review February 27, 2024 07:26
new AxiosURLSearchParams(params, options).toString(_encode);
}

if (serializedParams) {

Choose a reason for hiding this comment

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

To avoid defining hashmarkIndex also when serializedParams in a falsy value, these line

  var hashmarkIndex = url.indexOf('#');
  if (hashmarkIndex !== -1) {
    url = url.slice(0, hashmarkIndex);
  }

can be moved inside the if

Copy link
Author

Choose a reason for hiding this comment

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

@JadeMugs I disagree. I don't think it matters much since the hashMark part of the code is only to strip out the URL. Besides, I'm following the version in #5113 here:

https://github.com/DigitalBrainJS/axios/blob/92f0758429541b05c614dc25bc712379d8a41dbd/lib/helpers/buildURL.js#L39-L57

@@ -22,7 +22,8 @@ const config: AxiosRequestConfig = {
params: { id: 12345 },
paramsSerializer: {
indexes: true,
encode: (value) => value
encode: (value) => value,
serialize: (value, options) => String(value)

Choose a reason for hiding this comment

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

Hey @briwa, I noticed the ParamsSerializerOptions type does not include the serialize prop.
The type should be aligned in the index.d.ts file.

Note: In v1.x, the ParamsSerializerOptions interface is defined like this:

export interface ParamsSerializerOptions extends SerializerOptions {
  encode?: ParamEncoder;
  serialize?: CustomParamsSerializer;
}

Copy link
Author

Choose a reason for hiding this comment

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

@JadeMugs Thanks, I missed it. I just added them

@JadeMugs
Copy link

Hey @briwa, watching your MR I see the implementation is slightly different from the one in v1.x. IMMO this choice could create more friction for users who try to upgrade Axios from v0.x to v1.x.

@briwa
Copy link
Author

briwa commented Mar 22, 2024

Hey @briwa, watching your MR I see the implementation is slightly different from the one in v1.x. IMMO this choice could create more friction for users who try to upgrade Axios from v0.x to v1.x.

@JadeMugs Hey, I disagree that it will cause friction because as I mentioned in the PR description, this PR is simply following the same patch that was introduced in #5113. If this patch would cause friction, then the original patch would be, too, since I'm not doing anything different. But I might be wrong, so I'll let the maintainers decide.

@briwa briwa requested a review from JadeMugs March 26, 2024 08:02
@DigitalBrainJS DigitalBrainJS merged commit 111811e into axios:v0.x Mar 28, 2024
7 checks passed
@briwa briwa deleted the backport-params-serializer-options branch March 29, 2024 03:17
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

3 participants