- Notifications
You must be signed in to change notification settings - Fork20.6k
Ajax: Support an alternative completeCallback API for transports#4634
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Closing & re-opening the PR to trigger the EasyCLA check... |
aaronliu0130 commentedJul 8, 2024
I like this! Would simplify callback functions a bit and make them look way less ugly. |
I'm in favor and curious the total size after switching all internal transports. |
Wow, 4 years ago, time flies. 😅 My last comment still stands, though - I treat this PR as a pre-requisite of#4405 but we need another one - with |
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
Summary
Apart from the existing API:
a new API is now available:
This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.
Refgh-4405
+37 bytes. Not changing existing transports would make that smaller but we'll need to change the XHR one anyway to land#4405 and then the build gets smaller if we update it in all the places I modified in the PR.
Note: This still needs tests. That said, we don't have any direct tests for
jQuery.ajaxTransport
, its only implicitly tested by the virtue of two core transports using this API. Therefore, before landing this I'd like to write a few tests for the API first. I'm opening this draft PR now to gain feedback whether this is a direction in which we'd like to go.Checklist