Skip to content

Simplification, clarify and update some of the ajax logic #2033

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.

Sign up for GitHub

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
Closed
markelog wants to merge 3 commits into from
Closed

Simplification, clarify and update some of the ajax logic #2033

markelog wants to merge 3 commits into from

Conversation

@markelog
Copy link
Member

@markelog markelog commented Jan 26, 2015

No description provided.

@markelog markelog added the Ajax label Jan 26, 2015
@gibson042
Copy link
Member

gibson042 commented Jan 26, 2015

Remove "type: GET" from "evalUrl" method
since "get" type is by default already

Anything that can be controlled by global ajaxSettings should be explicit here.

@dmethvin
Copy link
Member

dmethvin commented Jan 26, 2015

Oh yeah, really good point. I've seen some strange problems caused by that in projects.

@markelog
Copy link
Member Author

markelog commented Jan 26, 2015

  1. We have this for more then two years now, no complains for far.
  2. We already warn people about it in the docs.
  3. if user does set, for example "POST" method, as a default (for some unknown reason), that would mean he wants to make request with this method.

@dmethvin
Copy link
Member

dmethvin commented Jan 27, 2015

Wait, won't the $.ajax call go away (or change to script transport) when gh-1895 lands? Why make that change now?

@markelog
Copy link
Member Author

markelog commented Jan 27, 2015

Nice catch!

Although i did only preliminary work on gh-1895, so i'm not sure how much we can change there. So it might only making async: true (removing this property that is) in evalUrl and change a bunch of tests, or removing this method entirely and rewriting manipulation module a bit and change a bunch of tests.

We can keep things separate, atomic, improve code at hand and deal with gh-1895 in relevant PR.

@markelog
Ajax: simplify two ajax calls
8240e48 
* Remove "async = true" from script transport since it was needed
  for FF < 4 and old Opera which we do not support anymore

* Add comment to "evalUrl" method on why "type" field should be explicit
markelog added a commit to markelog/jquery that referenced this pull request Feb 10, 2015
@markelog
Ajax: make jQuery#load "type" field explicit
0416344 
Ref trac-11264
Closes jquerygh-2033
markelog added a commit to markelog/jquery that referenced this pull request Feb 10, 2015
@markelog
Ajax: make jQuery#load "type" field explicit
7720962 
* Move "evalScript.php" file to appropriate place

* Make jQuery#load "type" field explicit and add test for it

Ref trac-11264
Closes jquerygh-2033
markelog added a commit to markelog/jquery that referenced this pull request Feb 10, 2015
@markelog
Ajax: make jQuery#load "type" field explicit
8dd81a7 
* Move "evalScript.php" file to appropriate place

* Make jQuery#load "type" field explicit and add test for it

Ref trac-11264
Closes jquerygh-2033
markelog added a commit to markelog/jquery that referenced this pull request Feb 10, 2015
@markelog
Ajax: make jQuery#load "type" field explicit
2366582 
* Move "evalScript.php" file to appropriate place

* Make jQuery#load "type" field explicit and add test for it

Ref trac-11264
Closes jquerygh-2033
@markelog
Copy link
Member Author

markelog commented Feb 10, 2015

@gibson042 comment is taken to the account, load is now explicitly uses GET method too. Test added.

markelog added a commit that referenced this pull request Feb 10, 2015
@markelog
Ajax: simplify two ajax calls
254c5ef 
* Remove "async = true" from script transport since it was needed
  for FF < 4 and old Opera which we do not support anymore

* Add comment to "evalUrl" method on why "type" field should be explicit

Closes gh-2033
Ref b76eb91e6117ea0e15c889556033270d9744002c
@markelog
Ajax: make jQuery#load "type" field explicit
45043f9 
* Move "evalScript.php" file to appropriate place

* Make jQuery#load "type" field explicit and add test for it

Ref trac-11264
markelog added a commit to markelog/jquery that referenced this pull request Feb 11, 2015
@markelog
Ajax: replace "jqXHR.complete" callback with "always"
059befe 
Since it was deprecated since 1.8

Closes jquerygh-2033
@markelog
Copy link
Member Author

markelog commented Feb 11, 2015

Added another commit that replaces jqXHR.complete in jQuery#load since it was deprecated since 1.8

gibson042
gibson042 reviewed Feb 11, 2015
View reviewed changes
src/ajax/load.js Outdated
Copy link
Member

@gibson042 gibson042 Feb 11, 2015

Choose a reason for hiding this comment

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

We could really use some comments here, because the code takes advantage of some tricky behavior. If the request succeeds, this function gets ( data, status, jqXHR ), but they are ignored because response was set above. If it fails, this function gets ( jqXHR, status, error ) (the first two of which coincide with complete) and are used in the body. It's probably also worth documenting that the self.each takes advantage of .each's undocumented second parameter.

@markelog
Ajax: replace "jqXHR.complete" callback with "always"
c93b902 
Since it was deprecated since 1.8.
Also add additional comments which explains tricky behaviour of always callback

Closes jquerygh-2033
@markelog
Copy link
Member Author

markelog commented Feb 13, 2015

Comment for always callback added.

jQuery#each second argument was always internal and each performance matter plus byte size and all that - that is the only place left that uses it.

@timmywil, @dmethvin @gibson042 i think you know where i'm going with this. How about it?

@dmethvin
Copy link
Member

dmethvin commented Feb 13, 2015

Yeah, i was wondering if we could just get rid of it when i saw the comment from @gibson042 earlier.

@timmywil
Copy link
Member

timmywil commented Feb 13, 2015

I LIKE it

@markelog
Copy link
Member Author

markelog commented Feb 13, 2015

Created - #2090

@markelog markelog changed the title Simplify two ajax calls Simplification, clarify and update some of the ajax logic Feb 14, 2015
markelog added a commit that referenced this pull request Feb 14, 2015
@markelog
Ajax: replace "jqXHR.complete" callback with "always"
fd80f59 
Since it was deprecated since 1.8.

Also add additional comments which explains tricky
behaviour of "always" callback

(cherry-picked from 97ef1f2)
Closes gh-2033
@markelog markelog closed this in 97ef1f2 Feb 14, 2015
markelog added a commit that referenced this pull request Nov 10, 2015
@markelog
Ajax: replace "jqXHR.complete" callback with "always"
aa87c91 
Since it was deprecated since 1.8.

Also add additional comments which explains tricky
behaviour of "always" callback

Closes gh-2033
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Ajax

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markelog @gibson042 @dmethvin @timmywil @jquerybot