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

Jump to bottom

Added custom params serializer support; #5113

Merged
merged 4 commits into from Oct 13, 2022
Merged

Added custom params serializer support; #5113

merged 4 commits into from Oct 13, 2022

Conversation

DigitalBrainJS
Copy link
Collaborator

@DigitalBrainJS DigitalBrainJS commented Oct 12, 2022 •
edited

Added paramsSerializer.serialize option to support custom params serializer as a workaround for #5094

github-code-scanning[bot]
github-code-scanning bot found potential problems Oct 12, 2022
View reviewed changes
lib/core/Axios.js Fixed Show fixed Hide fixed
@ChronosMasterOfAllTime
Copy link
Contributor

ChronosMasterOfAllTime commented Oct 12, 2022 •
edited

@DigitalBrainJS is this duplicative effort of #5108 ?

EDIT: I also found a bug with null values being included in the serialized params list and resolved those in the PR. TDD FTW

@DigitalBrainJS
Added missed semicolon;
aa332dc 
Fixed allowUnknown option;
jasonsaayman
jasonsaayman approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@jasonsaayman jasonsaayman left a comment

Choose a reason for hiding this comment

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

Looks good thanks 👍

@jasonsaayman jasonsaayman merged commit 9d4b016 into axios:v1.x Oct 13, 2022
7 checks passed
@bertho-zero
Copy link

Hi,
Can you release a version with this fix? many of us can't upgrade to v1 without it

@ChronosMasterOfAllTime
Copy link
Contributor

ChronosMasterOfAllTime commented Oct 14, 2022 •
edited

Hi,
Can you release a version with this fix? many of us can't upgrade to v1 without it

Interestingly this PR overlooks a couple of bugs

  • slicing off the hash from the URL regardless of serializedParams being valid or undefined. Old behavior was inside of the if statement

  • nulls still get added to the URL

I already had a fix for this exact thing (see #5108) but it still sits open. It allowed custom params serialzers based on a boolean flag and made use of the existing encode field. Even added unit tests for all these behaviors. Not sure why we're duplicating effort here. 🤷

EDIT: PR updated with bug fixes after merging this in

@Roriz
Copy link

Roriz commented Oct 17, 2022

Hey! This feature made a breaking change on paramsSerializer option, we should change a major version for that please

@Roriz Roriz mentioned this pull request Oct 17, 2022
Closed
@DigitalBrainJS
Copy link
Collaborator Author

DigitalBrainJS commented Oct 17, 2022 •
edited

Hey! This feature made a breaking change on paramsSerializer option, we should change a major version for that please

@Roriz No, the paramsSerializer option has been changed in v1.0.0-alpha.1 as intended. This PR only adds an assertion for the parameter, because non-TypeScript users may continue to use the old interface by passing in an erroneous option value, which will be ignored by Axios. The addition of the assertion is not a breaking change.

@Roriz
Copy link

Roriz commented Oct 17, 2022

As you can see on issue #5142, from this PR forward we can't use the old interface for paramsSerializer

Roriz
Roriz reviewed Oct 17, 2022
View reviewed changes
lib/core/Axios.js
@@ -57,6 +57,13 @@ class Axios {
}, false);
}

if (paramsSerializer !== undefined) {
validator.assertOptions(paramsSerializer, {
Copy link

@Roriz Roriz Oct 17, 2022 •
edited

Choose a reason for hiding this comment

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

I believe that this new validations creates a breaking change

@DigitalBrainJS
Copy link
Collaborator Author

we can't use the old interface for paramsSerializer

Yes, the old interface has been refactored in v1.0.0-alpha.1, in other words, the old interface is not supported since v1.0.0-alpha.1. Please check the index.d.ts file to see the new paramsSerializer interface definition.
This PR doesn't change the option behavior, it adds runtime type checking.

@Roriz
Copy link

Roriz commented Oct 17, 2022 •
edited

Ok, we changed the interface on alpha.1, but we don't add any warning about the change in the runtime. And now we put a validation check in runtime, any application that uses this config will break without alarm.
Here where I work, we only discover after deploying production and creates a downtime =/

@J3m5
Copy link

J3m5 commented Oct 17, 2022

I see three problems here.
The new interface use the same name as the previous one.
A validation that throw an error instead of logging a warning has been added in a patch version.
The docs hasn't been updated in the meantime.

@pegiadise pegiadise mentioned this pull request Oct 25, 2022
Closed
@ghost ghost mentioned this pull request Nov 3, 2022
Open
@girishjaju18 girishjaju18 mentioned this pull request Nov 22, 2022
Closed
@remidej remidej mentioned this pull request Jan 13, 2023
Merged
@manonpalin manonpalin mentioned this pull request Mar 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roriz Roriz Roriz left review comments

@jasonsaayman jasonsaayman jasonsaayman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@DigitalBrainJS @ChronosMasterOfAllTime @bertho-zero @Roriz @J3m5 @jasonsaayman