Closed
Bug 1186396
Opened 9 years ago
Closed 9 years ago
Drop Loop's use of jQuery from the standalone UI
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox43 fixed)
People
(Reporter: standard8, Assigned: dcritchley, Mentored)
References
Details
(Whiteboard: [tech-debt][lang=js])
User Story
1) get rid of jQuery usage in webapp.jsx 2) replace $.ajax calls with raw XHR invocations in standaloneMozLoop.js 3) replace $.ajax and $.get call with raw XHR invocations in standaloneClient.js 4) get rid of jQuery loading from content/shared/libs to ui/ (maybe instead content/js/libs -- what about backbone dependency?) 5) Remove the loading of jquery from browser/components/loop/standalone/index.html, as well as browser/components/loop/test/standalone/index.html spin off into another bug: ?) hoist raw XHR invocations into common function/object? ?) make mixins.js audio creation use common function/object?
Attachments
(1 file, 4 obsolete files)
280.40 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
In bug 1186362 we dropped the use of jQuery from the Loop desktop side. We should be able to do the same for the standalone side as well, but this needs a little more work: - webapp.jsx currently references it in the function enclosue, but doesn't need it. - standaloneClient.js and standaloneMozLoop.js both use $.get or $.ajax, these need to be changed to use XMLHttpRequest [1] - Remove the loading of jquery from browser/components/loop/standalone/index.html, as well as browser/components/loop/test/standalone/index.html - Move jquery from the content/shared/libs/ directory to ui/ For background information on Loop development see https://wiki.mozilla.org/Loop/Development, especially https://wiki.mozilla.org/Loop/Development#Standalone_server_-_Link-clicker_UI_and_unit_test_development [1] https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest
Updated•9 years ago
|
Points: --- → 3
User Story: (updated)
Updated•9 years ago
|
Assignee: nobody → david.critchley
Flags: firefox-backlog+
Comment 1•9 years ago
|
||
Standard8: is this expected to cause any issues with Backbone on standalone, since we depend on that? It looks like our use there is completely minimal, so I'm sort of guessing not. I'm also assuming that we'll want to leave Backbone in place for now since we're using it for events to. Thoughts?
Flags: needinfo?(standard8)
Reporter | ||
Comment 2•9 years ago
|
||
The backbone homepage states: "Backbone's only hard dependency is Underscore.js ( >= 1.7.0). For RESTful persistence and DOM manipulation with Backbone.View, include jQuery ( >= 1.11.0), and json2.js for older Internet Explorer support." Hence, jQuery is only used for the View stuff, and we're not using any views. (Note, we use lodash rather than underscore, but that's acceptable). Like you say Backbone is still needed for Events and some of the old call models (which I'm hoping will go soon), so whilst I do want to replace it, I think we'll be doing that a bit later in a separate bug (we need a library or custom code with which to replace Backbone's events - I have some ideas, just not a priority atm).
Flags: needinfo?(standard8)
Updated•9 years ago
|
Iteration: --- → 42.3 - Aug 10
Rank: 25
Priority: -- → P2
Comment 3•9 years ago
|
||
The hoisting _might_ want to be done using the Fetch polyfill mentioned on <https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch>, but, more likely, we should just spin that out to a separate bug.
Updated•9 years ago
|
Iteration: 42.3 - Aug 10 → ---
Assignee | ||
Comment 4•9 years ago
|
||
Loop, removing references to JQuery
Attachment #8653204 -
Flags: review?(andrei.br92)
Assignee | ||
Comment 5•9 years ago
|
||
Wanting to know where I should move jquery-2.1.4.js to. Options: /ui content/js * content/libs I would choose the location with the *, but would like more info or a firm decision before I go ahead with this plan.
Flags: needinfo?(standard8)
Reporter | ||
Comment 6•9 years ago
|
||
(In reply to David Critchley (:dcritch) from comment #5) > Wanting to know where I should move jquery-2.1.4.js to. > > Options: > /ui > content/js > * content/libs > > I would choose the location with the *, but would like more info or a firm > decision before I go ahead with this plan. Its not used in desktop now, so lets just go with /ui. I don't think we need a libs directory (I'd like to drop it completely sooner or later anyway). Can you also remove the jquery references from test/karma/karma.coverage.desktop.js and test/karma/karma.coverage.shared_standalone.js please? They shouldn't be necessary any more (and are new since this bug was filed).
Flags: needinfo?(standard8)
Comment 7•9 years ago
|
||
Comment on attachment 8653204 [details] [diff] [review] Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI Review of attachment 8653204 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, I just had some comments about the way we handle the options that get sent in at one point. ::: browser/components/loop/standalone/content/js/standaloneClient.js @@ +11,5 @@ > > /** > * Loop server standalone client. > * > + * @param {Object} options object. Why did this change from being called `settings` to `options`? @@ +20,4 @@ > throw new Error("missing required baseServerUrl"); > } > > + this._baseServerUrl = options.baseServerUrl; Why aren't we attaching all settings that get passed in anymore? @@ +70,5 @@ > > cb(err); > }, > > + /** This changed. It should align on the `*`. ::: browser/components/loop/standalone/content/js/standaloneMozLoop.js @@ +45,2 @@ > err.errno = jsonErr.errno; > + Whitespace. @@ +186,5 @@ > * @param {Function} callback Function that will be invoked once the operation > * finished. The first argument passed will be an > * `Error` object or `null`. > */ > + refreshMembership: function(roomToken, sessionToken, callback) { This method is indented inward.
Attachment #8653204 -
Flags: review?(andrei.br92) → review-
Assignee | ||
Comment 8•9 years ago
|
||
Loop, removing references to JQuery
Attachment #8653204 -
Attachment is obsolete: true
Attachment #8653671 -
Flags: review?(standard8)
Attachment #8653671 -
Flags: review?(andrei.br92)
Assignee | ||
Comment 9•9 years ago
|
||
Loop, removing references to JQuery
Attachment #8653671 -
Attachment is obsolete: true
Attachment #8653671 -
Flags: review?(standard8)
Attachment #8653671 -
Flags: review?(andrei.br92)
Attachment #8653674 -
Flags: review?(standard8)
Attachment #8653674 -
Flags: review?(andrei.br92)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8653674 [details] [diff] [review] Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI Review of attachment 8653674 [details] [diff] [review]: ----------------------------------------------------------------- Assuming you do remove jquery as mentioned below, then please can you also remove the "jQuery" and "$" entries from the globals section of /browser/components/loop/.eslintrc ::: browser/components/loop/ui/ui-showcase.jsx @@ +1684,5 @@ > // This simulates the mocha layout for errors which means we can run > // this alongside our other unit tests but use the same harness. > var expectedWarningsCount = 10; > var warningsMismatch = caughtWarnings.length !== expectedWarningsCount; > + var resultsElement = document.querySelector("#results"); If we're doing this, then we can drop jquery completely. I'm quite happy changing with it being dropped from here, but what do you think about doing this in a React view? I'm pretty sure you could do something like: var ResultsView = React.createClass({ propTypes: ..., render: function() { <div className="#failures"> {this._renderFailures()} </div> <p id="complete">Complete</p> } }); React.renderComponent(<ResultsView />, document.getElementById("results")); I think that would be a bit clearer overall, and easier for future maintenance.
Attachment #8653674 -
Flags: review?(standard8)
Attachment #8653674 -
Flags: review?(andrei.br92)
Attachment #8653674 -
Flags: review-
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #10) > Comment on attachment 8653674 [details] [diff] [review] > Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI > > Review of attachment 8653674 [details] [diff] [review]: > ----------------------------------------------------------------- > > Assuming you do remove jquery as mentioned below, then please can you also > remove the "jQuery" and "$" entries from the globals section of > /browser/components/loop/.eslintrc > > ::: browser/components/loop/ui/ui-showcase.jsx > @@ +1684,5 @@ > > // This simulates the mocha layout for errors which means we can run > > // this alongside our other unit tests but use the same harness. > > var expectedWarningsCount = 10; > > var warningsMismatch = caughtWarnings.length !== expectedWarningsCount; > > + var resultsElement = document.querySelector("#results"); > > If we're doing this, then we can drop jquery completely. I'm quite happy > changing with it being dropped from here, but what do you think about doing > this in a React view? I'm pretty sure you could do something like: > > var ResultsView = React.createClass({ > propTypes: ..., > > render: function() { > <div className="#failures"> > {this._renderFailures()} > </div> > <p id="complete">Complete</p> > } > }); > > React.renderComponent(<ResultsView />, document.getElementById("results")); > > I think that would be a bit clearer overall, and easier for future > maintenance. We were trying to keep the scope down for this bug. I will add another bug for changing this to a React Component.
Assignee | ||
Comment 12•9 years ago
|
||
Loop, removing references to JQuery
Attachment #8653674 -
Attachment is obsolete: true
Attachment #8654273 -
Flags: review?(andrei.br92)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to David Critchley (:dcritch) from comment #11) > (In reply to Mark Banner (:standard8) from comment #10) > > Comment on attachment 8653674 [details] [diff] [review] > > Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI > > > > Review of attachment 8653674 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Assuming you do remove jquery as mentioned below, then please can you also > > remove the "jQuery" and "$" entries from the globals section of > > /browser/components/loop/.eslintrc > > > > ::: browser/components/loop/ui/ui-showcase.jsx > > @@ +1684,5 @@ > > > // This simulates the mocha layout for errors which means we can run > > > // this alongside our other unit tests but use the same harness. > > > var expectedWarningsCount = 10; > > > var warningsMismatch = caughtWarnings.length !== expectedWarningsCount; > > > + var resultsElement = document.querySelector("#results"); > > > > If we're doing this, then we can drop jquery completely. I'm quite happy > > changing with it being dropped from here, but what do you think about doing > > this in a React view? I'm pretty sure you could do something like: > > > > var ResultsView = React.createClass({ > > propTypes: ..., > > > > render: function() { > > <div className="#failures"> > > {this._renderFailures()} > > </div> > > <p id="complete">Complete</p> > > } > > }); > > > > React.renderComponent(<ResultsView />, document.getElementById("results")); > > > > I think that would be a bit clearer overall, and easier for future > > maintenance. > > We were trying to keep the scope down for this bug. I will add another bug > for changing this to a React Component. https://bugzilla.mozilla.org/show_bug.cgi?id=1199815
Comment 14•9 years ago
|
||
Comment on attachment 8654273 [details] [diff] [review] Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI Review of attachment 8654273 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!
Attachment #8654273 -
Flags: review?(andrei.br92) → review+
Comment 16•9 years ago
|
||
It is only included in the UI showcase but this patch removes its usage there. Can we go ahead and remove the library?
Reporter | ||
Comment 17•9 years ago
|
||
Yes we should be. The patch needs a simple change to remove the inclusion in ui/index.html, and drop the .eslintignore change. I was trying to suggest this in comment 10, but i think it got entangled in the suggesrion to move the revised bit to react.
Flags: needinfo?(standard8)
Assignee | ||
Comment 19•9 years ago
|
||
Loop, removing references to JQuery
Attachment #8654273 -
Attachment is obsolete: true
Attachment #8655626 -
Flags: review?(standard8)
Attachment #8655626 -
Flags: review?(andrei.br92)
Reporter | ||
Comment 20•9 years ago
|
||
Comment on attachment 8655626 [details] [diff] [review] Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI Review of attachment 8655626 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, as Andrei has already reviewed the most of the patch, I'm just r+ the inter diff here.
Attachment #8655626 -
Flags: review?(standard8)
Attachment #8655626 -
Flags: review?(andrei.br92)
Attachment #8655626 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(david.critchley)
Reporter | ||
Updated•9 years ago
|
Iteration: --- → 43.2 - Sep 7
Flags: qe-verify-
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b9c136a827c5
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•