patternfly / patternfly-bootstrap-treeview

Tree View for Twitter Bootstrap -
http://jonmiles.github.io/bootstrap-treeview
Apache License 2.0
200 stars 105 forks source link

Make the library compatible with jQuery v3 #65

Open abkieling opened 6 years ago

abkieling commented 6 years ago

Make the library compatible with jQuery v3.

skateman commented 6 years ago

Hmm, I am not against it, but we depend on this project in ManageIQ and this might break stuff. What do you think @himdel || @karelhala?

himdel commented 6 years ago

As long as "make compatible with jquery 3" doesn't mean "break compatibility with previous releases". :)

@akieling What's missing here is any kind of indication why this would not be compatible now, and what kind of changes you need done for it to be compatible.

(Frankly I'm surprised this is not compatible already, are we using some deprecated function somewhere?)

karelhala commented 6 years ago

Well we are using >= for jQuery in bower.json#L29 so while testing it might download jQuery 3 without us knowing it. Because https://docs.npmjs.com/misc/semver, so same question as @himdel's are we using something from old jquery we shouldn't?

abkieling commented 6 years ago

The tests are using jQuery v2. Have a look at ./tests/tests.html. It references ./tests/lib/jquery.js, not the one installed by Bower or npm. When you update it to v3, most tests break.

On 29 Jan 2018 11:40 am, "Karel Hala" notifications@github.com wrote:

Well we are using >= for jQuery in bower.json#L29 https://github.com/jonmiles/bootstrap-treeview/blob/master/bower.json#L29 so while testing it might download jQuery 3 without us knowing it. Because https://docs.npmjs.com/misc/semver, so same question as @himdel https://github.com/himdel's are we using something from old jquery we shouldn't?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/patternfly/patternfly-bootstrap-treeview/issues/65#issuecomment-361248047, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0lT9hqpfkccV6ksSNgUC8_9eHAAZmjks5tPco-gaJpZM4RuW4_ .

himdel commented 6 years ago

Looks like the relevant failure is this one:

$ grunt test
Running "qunit:all" (qunit) task
Testing tests/tests.html .FFF.FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
>> PhantomJS timed out, possibly due to:
>> - QUnit is not loaded correctly.
>> - A missing QUnit start() call.
>> - Or, a misconfiguration of this task.
Warning: 1 tests completed with 1 failed, 0 skipped, and 0 todo. 
0 assertions (in 0ms), passed: 0, failed: 0 Use --force to continue.

that looks more like qunit isn't starting properly than actual failures.

Looks like the tests need to be fixed to use a current version of qunit and jquery, and not to bundle these things in the repo.

So far, I don't see any evidence of patternfly-bootstrap-treeview not working with jQuery 3.

himdel commented 6 years ago

Trying the demo, it seems to work, though I am getting this exception:

jquery.js:3818 jQuery.Deferred exception: Cannot read property 'length' of undefined TypeError: Cannot read property 'length' of undefined
    at Function.grep (http://localhost:3000/bower_components/jquery/dist/jquery.js:418:19)
    at Tree._findNodes (http://localhost:3000/js/bootstrap-treeview.js:1874:12)
    at Tree._getSearchResults (http://localhost:3000/js/bootstrap-treeview.js:1851:15)
    at Tree.search (http://localhost:3000/js/bootstrap-treeview.js:1800:23)
    at Object.proxy [as search] (http://localhost:3000/bower_components/jquery/dist/jquery.js:10268:13)
    at HTMLDivElement.<anonymous> (http://localhost:3000/js/bootstrap-treeview.js:1934:30)
    at Function.each (http://localhost:3000/bower_components/jquery/dist/jquery.js:354:19)
    at jQuery.fn.init.each (http://localhost:3000/bower_components/jquery/dist/jquery.js:189:17)
    at jQuery.fn.init.$.fn.(anonymous function) [as treeview] (http://localhost:3000/js/bootstrap-treeview.js:1921:8)
    at findSelectableNodes (http://localhost:3000/:571:34) undefined
himdel commented 6 years ago

... and that's probably a timing bug .. jQuery 3 changed their promise-like thing to behave according to Promise specs, meaning that done should never happen synchronously.

So presumably the this._orderedNodes = this._sortNodes() assignment is now happening later than that return $.grep(this._orderedNodes, $.proxy(function (node) {.

abkieling commented 6 years ago

I'm getting the same error when upgrading one of the hawt.io projects to jQuery 3. Has anyone tried running the tests with the latest versions of jQuery and qunit? I can try that next week.

On 30 Jan 2018 11:24 am, "Martin Hradil" notifications@github.com wrote:

... and that's probably a timing bug .. jQuery 3 changed their promise-like thing to behave according to Promise specs, meaning that done should never happen synchronously.

So presumably the this._orderedNodes = this._sortNodes() assignment is now happening later than that return $.grep(this._orderedNodes, $.proxy(function (node) {.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/patternfly/patternfly-bootstrap-treeview/issues/65#issuecomment-361592096, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0lT0QhV7kfikp_LQVKyLAJ5pnnz85Iks5tPxghgaJpZM4RuW4_ .

himdel commented 6 years ago

@akieling I.. tried to try :) But looks like the tests need a complete rewrite to work with current versions of qUnit.

If you can get them working, a PR would be much appreciated :). (Preferrably so that we don't have to bundle qunit and jquery in the repo anymore, but we can take care of that.)