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

Is decodeDotInKeys meant to be disabled by default? #500

Closed
wwaaijer-exh opened this issue Apr 3, 2024 · 1 comment · Fixed by #501
Closed

Is decodeDotInKeys meant to be disabled by default? #500

wwaaijer-exh opened this issue Apr 3, 2024 · 1 comment · Fixed by #501

Comments

@wwaaijer-exh
Copy link
Contributor

Currently decodeDotInKeys is enabled by default, was this intentional?

We use qs for oAuth1 signature generation, for which it is important to keep keys as they are.
By having decodeDotInKeys automatically enabled by default in 6.12.0, keys are implicitly transformed.

We did not pin the exact version of qs, just the major version.
The moment 6.12.0 was released, new installations started to break.
Following that reasoning, could this be considered a breaking change?

All not that bad though, just something we did not expect to happen.
We're still very satisfied qs users!

As a side note, the examples in the docs show how to enable decodeDotInKeys, implying it is disabled by default.

I'm happy to create a PR to either update the default behavior or change the docs.

@ljharb
Copy link
Owner

ljharb commented Apr 4, 2024

I see that it is enabled by default - https://github.com/ljharb/qs/blob/main/lib/parse.js#L17C5-L17C20, and comparing https://github.com/ljharb/qs/blob/main/lib/parse.js#L132 to https://github.com/ljharb/qs/blob/v6.11.2/lib/parse.js#L157, it does seem like this is an unintentional breaking change.

A PR to restore the default and update the relevant test cases (so that the docs are correct) would be very appreciated!

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 a pull request may close this issue.

2 participants