Skip to content

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

Merged
DigitalBrainJS merged 4 commits into
axios:v0.xfrom
briwa:backport-params-serializer-options
Mar 28, 2024
Merged

backport: custom params serializer support for v0.x#6263
DigitalBrainJS merged 4 commits into
axios:v0.xfrom
briwa:backport-params-serializer-options

Conversation

@briwa

@briwa briwa commented Feb 27, 2024

Copy link
Copy Markdown

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
Comment thread lib/helpers/buildURL.js
new AxiosURLSearchParams(params, options).toString(_encode);
}

if (serializedParams) {

Copy link
Copy Markdown

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
Copy Markdown
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

Comment thread test/typescript/axios.ts
indexes: true,
encode: (value) => value
encode: (value) => value,
serialize: (value, options) => String(value)

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

briwa commented Mar 22, 2024

Copy link
Copy Markdown
Author

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
@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.

3 participants