Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Upgrade jQuery #3072

Closed
TuckerWhitehouse opened this issue Mar 8, 2013 · 9 comments
Closed

Upgrade jQuery #3072

TuckerWhitehouse opened this issue Mar 8, 2013 · 9 comments

Comments

@TuckerWhitehouse
Copy link
Contributor

I'm not sure if it's been discussed at all, but Brackets is still on jQuery 1.7 and there would be some speed and stability benefits of upgrading to jQuery 1.9.1.

Unfortunately, some deprecated functions are still being used in core so the jQuery migrate plugin would need to be included as well.

I'm not sure what the intended browser support is for Brackets, but jQuery 2.0 may also be a good option (decreased size, deprecated functions removed, customizable), but drops support for IE 6, 7 & 8.

I attempted to run the unit tests after swapping out jQuery-1.7 with jQuery-1.9.1 and including the jQuery Migrate plugin, but unit tests are failing for other reasons right now (#3070).

@peterflynn
Copy link
Member

@TuckerWhitehouse can you list out which deprecated functions we're using? That would be a good start to cleaning up the code -- ideally we'd do a clean migration instead of relying on a shim.

@TuckerWhitehouse
Copy link
Contributor Author

Certainly, it'll be a little bit easier once the unit tests bug is solved.

I know one thing was the Node Connection is using the deferred.isRejected() which was deprecated in 1.7 and removed in 1.8 (http://api.jquery.com/deferred.isRejected/), should be using deferred.state()==="rejected"

Edit:
A quick "Find in Files" for ".isRejected" shows that it is actually used in

  • editor/CodeHintManager.js (line 344)
  • utils/NodeConnection.js (line73)
  • LiveDevelopment/Inspector/Inspector.js (line 269)
  • extensions/default/HTMLCodeHints/main.js (lines 290, 299 & 305)

I can go through and start replacing these and submit a pull request

@pthiess
Copy link
Contributor

pthiess commented Mar 11, 2013

Reviewed, we thought it was ok to make changes and add a pull request to support 1.9.1 but only if it wouldn't break anything in 1.7. Changing our implementation basis would require significant testing, therefore we move it to backlog. Added card 813. Closing in here eventually.

@adrocknaphobia
Copy link
Contributor

I'm not convinced this should get added to the backlog (yet). It seems as if the work to upgrade jQuery is simple enough (replacing any deprecated API calls or implementing the migrate plugin). The real effort would be around testing.

I'd like to propose that we leave this issue open for @TuckerWhitehouse (or others) to work on, but when the pull request is ready and extensive testing is required we will add it to the backlog.

I think we should also upgrade directly to 2.0 (and skip 1.9.1).

@TuckerWhitehouse
Copy link
Contributor Author

I have a branch where i'm making all the changes (mostly just the use of .isResolved, .isRejected, .andSelf and $.browser). I had almost everything done, but left the "Whitespace Normalizer" extension enabled... so 1000's of lines got added to the commits for whitespace :/

Testing was successful for everything (unit, integration, etc) except the Inline Code Editor Extension (not sure why that was failing exactly... the errors had nothing to do with jQuery)

As for version, 1.9.1 and 2.0 both have the same api, but 2.0 is still in beta.

Should have a pull request in the next few hours.

@TuckerWhitehouse
Copy link
Contributor Author

My Branch: https://github.com/TuckerWhitehouse/brackets/compare/update_jQuery

jQuery 2.0.0b2 has some bugs, so we have to use 1.9.1 (for now).
4 ProjectManager tests and 1 WorkingSetView test fail on 2.0.0b2

The Bootstrap plugins use $.browser which is deprecated. Would it be better to polyfill $.browser, or look into upgrading these plugins to the latest versions which no longer rely on $.browser?


The Jasmine Spec runner fails to load 2 files. (In the Jasmine Window, Right Click, and Choose Show DevTools).
"test/LiveDevelopment/Inspector/Inspector.json" included from "src/LiveDevelopment/Inspector/Inspector.js"
"test/LiveDevelopment/main.less" included from "src/LiveDevelopment/main.js"

Both of these files are included using a $.get, and the root directory is different in the test window.

8 of the Inline Color Editor tests fail (that have nothing to do with jQuery as far as I can see...)

@TuckerWhitehouse
Copy link
Contributor Author

For the Jasmine Spec runner load failures, I had to modify the Inspector.js and main.js files (see previous comment), and replace the $.get's with require's, and that problem is now fixed.

Still seeing 8 failures with the Inline Color Editor tests :/

@pthiess
Copy link
Contributor

pthiess commented Mar 12, 2013

@adrocknaphobia Ok, this is exactly why we left it open - usually we would close an item after we moved it to Trello. We can pull the card from the icebox whenever you decide to make it a priority.

@peterflynn
Copy link
Member

Per the comments in #3123, we have this tracked in the backlog now, so I'll close this bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants