parse-community / Parse-SDK-JS

The JavaScript SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
1.31k stars 599 forks source link

feat: Enable `Parse.idempotency` by default #2164

Closed dplewis closed 1 month ago

dplewis commented 1 month ago

Pull Request

Issue

There isn't a well documented way to turn idempotency on, enabling it by default only requires configuration on the server side. This is how it is handled in other SDKs

https://github.com/parse-community/Parse-Swift/pull/62 https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/1790 https://github.com/parse-community/Parse-SDK-Android/pull/1190

Approach

Tasks

parse-github-assistant[bot] commented 1 month ago

Thanks for opening this pull request!

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (df6df7c) to head (6ac20ca). Report is 1 commits behind head on alpha.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## alpha #2164 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 64 64 Lines 6364 6363 -1 Branches 1507 1509 +2 ========================================= - Hits 6364 6363 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mtrezza commented 1 month ago
dplewis commented 1 month ago

This should have been enabled from the beginning. This is the default behavior of the other SDKs as I mentioned in the OP.

I wouldn't consider this a breaking change as wouldn't cause any disruptions to the developers. Adding this request header by default is pretty much useless unless the feature is enabled on the server. If it’s enabled on the server already it’s likely enabled in the SDK

A breaking change would be removing the ability to turn it off.

mtrezza commented 1 month ago

I'm not sure it should be on by default. That's at least not what I had in mind when I designed the feature. It's costly on server side resources when it's enabled on the server and a minority of users is likely using it. Sending a UID by default from the client to the server without the server using it would not be good practice because that generates unnecessary costs.

It could be breaking considering that we change the request headers. An additional header may cause a request to be rejected on tightly configured firewalls. It's like changing a request HTTP method, that would also be a breaking change for the same reason.

dplewis commented 1 month ago

@mtrezza Im closing this as it works for me. From a client side cost side we should purge the other SDKs headers as well from the parse.com days. Can you add documentation so developers can enable this feature since designed it?

mtrezza commented 1 month ago

I agree that other client SDKs should also not send the idempotency header by default. For docs see here.