Permalink
5 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Showing
1 changed file
with
5 additions
and
5 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
639f493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "old load method" defined anywhere? I don't see it. It looks like if there is no old "load" method, and the first argument is a string, a TypeError should occur (as the result of performing a call operation on an undefined value).
Have you considered moving some of the method to a separate function, e.g. from:
to:
I also see that jQuery.ajax is a very long method, with a lot of conditional logic around the
dataType
.639f493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, should be:
...if there is no "old load" method, and the first argument is not a string...
639f493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "old load method" is in event.js, which is always compiled in before ajax.js.
639f493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see: This is a workaround to an internal name collision of
load
. The workaround/solution you've employed seems to be creating very tight coupling between ajax.js, event.js, and build order.The
load
method in ajax.js expects that there is aload
method defined elsewhere (it is in event.js), that that otherload
method accepts non-string values and that it does not accept string values (because ajax.js'load
takes that case). This is essentially a mutual exclusion between parameter type between two same-named methods of two modules.The problems related to the name collision can be avoided by renaming ajax.js'
load
method to something that does not conflict, such asloadURL
,makeRequest
, etc. Why not do that?639f493
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we've done it this way for almost 4 years now. The decision to overload the functionality of .load() to both event and ajax was made fairly early on. Much in the same way we overload .attr() (and don't have getAttr and setAttr).