Possible bug in core (global.js)?

Jonathan W

So I couldn't figure out why my add-on wouldn't function as I expected under a clean install of Vanilla, and in my poking around I had found something.

https://github.com/vanilla/vanilla/blob/master/js/global.js#L679

      $.ajax({
         type: "POST",
         url: href,
         data: { DeliveryType: 'VIEW', DeliveryMethod: 'JSON', TransientKey: gdn.definition('TransientKey') },
         dataType: 'json',
         complete: function() {
            gdn.enable(this); // This is line #679
            $elem.removeClass(progressClass);
            $elem.attr('href', href);

            // If we are in a flyout, close it.
            $flyout.removeClass('Open').find('.Flyout').hide();
         },

The code above shows gdn.enable(this) where I believe gdn.enable($elem) should be used. The reason for this is because this refers to $.ajax in this context, where as $elem refers to the element that was clicked on. Earlier in the code, there is a gdn.disable(this), and here this is referring to the element in question. I've included a data dump of the two variables at the time line #679 is being parsed.

I'm more asking for those veteran devs to take a look at this and either validate it as a bug that should be fixed with a pull request, or no - it's not a bug, you're just doing it wrong.

The real strange thing is that my add-on works when I use the Bootstrap theme, and that this would appear to cause issues for many things, but it does not. I haven't looked too deep yet.

Some debugger nonsense!

this = Object {url: "/forums/discussion/close?discussionid=1&close=1", type: "POST", isLocal: false, global: true, processData: true…}
accepts: Object
async: true
complete: function () {
contentType: "application/x-www-form-urlencoded; charset=UTF-8"
contents: Object
converters: Object
crossDomain: false
data: "DeliveryType=VIEW&DeliveryMethod=JSON&TransientKey=Q10IRLP86FG3"
dataType: "json"
dataTypes: Array[2]
error: function (xhr) {
flatOptions: Object
global: true
hasContent: true
isLocal: false
jsonp: "callback"
jsonpCallback: function (){var e=Fn.pop()||x.expando+"_"+vn++;return this[e]=!0,e}
processData: true
responseFields: Object
success: function (json) {
type: "POST"
url: "/forums/discussion/close?discussionid=1&close=1"
xhr: function In(){try{return new e.XMLHttpRequest}catch(t){}}
__proto__: Object

And

$elem = [a.CloseDiscussion.Hijack.InProgress, context: a.CloseDiscussion.Hijack.InProgress, jquery: "1.10.2", constructor: function, init: function, selector: ""…]
0: a.CloseDiscussion.Hijack.InProgress
context: a.CloseDiscussion.Hijack.InProgress
length: 1
__proto__: x[0]

vrijvlinder

You need to understand what $this means in the context of the functions you want $this to be applied to. It could be anything .

So $this is an arbitrary term an anonymous term to call a specific function.

$this refers to the current object it can be any object.

http://php.net/manual/en/functions.anonymous.php

http://php.net/manual/en/function.call-user-func.php

Linc

I believe the code is correct as-is. In this context, $.ajax() is a function call, not a definition or a new context. So inside the $.ajax() call, (and even inside the closure beginning the line above) this refers to the same context as prior to the call.

Jonathan W

@vrijvlinder - I know exactly what this refers to in that code above. Look at it. $.ajax is an object for the request. The functions present for complete success error are all within the context of $.ajax, which means this refers to $.ajax. They are essentially anonymous member functions called from the scope of $.ajax. That's why I think this is a bug. Unless there's some reason for gdn.enable to be used on an AJAX request, it should be directed toward $elem.

If you can see a reason for gdn.enable to be used on the AJAX request, then please post the code here so we can further the discussion and I can learn. I hope I'm not coming off as snarky, but I do have a very solid grasp on object-oriented programming and the logic behind it. I'm a tad rusty, but I had years of Java, C++, C#, and so on under my belt.

Also, to update - I did more experimenting. I still don't know why my Hide Comment/Unhide Comment menu items (along with other menu items) are acting as if they are enabled after being clicked, but I can assure you that they are not enabled. The disabled="disabled" is still present on my Vanilla 2.1.9 forums at testing.wurmly.com - The only thing I can figure is something in the theme is overriding it being disabled in CSS.

Jonathan W

@Linc said:
I believe the code is correct as-is. In this context, $.ajax() is a function call, not a definition or a new context. So inside the $.ajax() call, (and even inside the closure beginning the line above) this refers to the same context as prior to the call.

But as I said above, isn't what we have there basically a declaration of an $.ajax object? complete: function() {}, is actually defining a member function called complete, so anything inside it would have scope local to the object it is defined in. That's also what my testing with Chrome's debugger has given me. That's why I pasted the debugger output in my original post. I put a break-point at line #679 and then copied the contents of those two local variables at that moment in the script's run time. It clearly shows that, at least on Chrome, it assumes this is referring to the object $.ajax.

I can't do more testing until later, but I'll grab Firefox and test it in Firebug and see if I get a different result. It's just when you break it down... $.ajax is an object definition. The method complete isn't being run at that very moment, it's asynchronous, so it's being called at a later point in the run time of the script.

Jonathan W

@Linc said:
I believe the code is correct as-is. In this context, $.ajax() is a function call, not a definition or a new context. So inside the $.ajax() call, (and even inside the closure beginning the line above) this refers to the same context as prior to the call.

I should clarify - yes $.ajax() is a function call, but one that creates an object that does a specific thing, and does it asynchronously. So as I said, complete becomes a member function of the object created by $.ajax, and when the reply comes back from the server, the object simply calls the member function complete. It is no longer in the same local scope as HijackClick, where this is equal to the element clicked.

Like I said on Twitter, I've spent a few hours on this. I'm new to the community, so I'm going to be scrutinized more. I like to make sure I'm positive on something, and I think I've provided enough proof to that effect here. What I'm looking for is equal proof to show I'm wrong, so I can learn. It's very possible that I've got my head stuck in the wrong hole here.

Linc

After reading the docs, I found that this is a reference to the settings themselves since no context was passed. So you are correct that its use there is wrong.

Jonathan W

Thanks for taking a look, @Linc ! I'm glad I'm not crazy. (Well, the avatar might prove otherwise, but oh well.)

I went ahead and created what might be the smallest pull request ever here:
https://github.com/vanilla/vanilla/pull/2726

Thanks for your help!

x00

good catch