vakata / jstree

jquery tree plugin
http://jstree.com
MIT License
5.15k stars 1.38k forks source link

Deprecated `Proxy` function on jQuery 3.3 #2492

Closed andtown closed 3 years ago

andtown commented 3 years ago

As of jQuery 3.3, the .proxy() has been deprecated and can be replaced with the native Function.prototype.bind method #2487

vakata commented 3 years ago

Sorry for the delay - I was wondering if your PR is OK the way it is (as it modifies the global scope with the polyfill). Then I realized way too much work would be involved to move the fix to the $.vakata scope, so I merged your changes. Thank you and sorry for the delay.

andtown commented 3 years ago

Thanks for merging the PR. Actually, I was thinking the same and was in two minds as to whether to move the fix to the $.vakata scope or create the bind prototype polyfill, however, I chose the polyfill as it was quite simple to implement and much less work to do. Is there any plan of releasing a new version (3.3.12) soon?

duzun commented 2 years ago

@andtown @vakata

I think there are too many $.proxy(), and now .bind() calls, in some places there is no need for a proxy function, in other places many proxy functions could be easily replaced with one variable like var inst = this in the parent scope.

Each .bind() creates a new intermediary function which adds some (negligible?) overhead in terms of one more function call and one more object to GC. When working with big amounts of data, these overheads sum up and might be noticeable (I did not benchmark).

Besides possible performance gains, there is also an aesthetic part. For eg.:

this.element
    .on("dblclick.jstree", ".jstree-anchor", function (e) {
        if(e.target.tagName && e.target.tagName.toLowerCase() === "input") { return true; }
        if(this.settings.core.dblclick_toggle) {
            this.toggle_node(e.target);
        }
    }.bind(this))

could be:

var inst = this;
inst.element
    .on("dblclick.jstree", ".jstree-anchor", function (e) {
        if(e.target.tagName && e.target.tagName.toLowerCase() === "input") { return true; }
        if(inst.settings.core.dblclick_toggle) {
            inst.toggle_node(e.target);
        }
    })

Also, when the code is minified, the this keyword is not mangled, but any other variable would probably get replaced with 1 letter variable, and the ~1500 this keywords used in jstree could become ~4500 less bytes of minified code.

Here are some of my attempts to partially optimize the code:

I'm using the first commit for some years now with no issues.

What are your thoughts on this?