Drop Loop's use of jQuery from the standalone UI

RESOLVED FIXED in Firefox 43

Status

Hello (Loop)
Client
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: dcritch, Mentored)

Tracking

unspecified
mozilla43
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox43 fixed)

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 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
Points: --- → 3
User Story: (updated)

Updated

3 years ago
Assignee: nobody → david.critchley
Flags: firefox-backlog+
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

3 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

3 years ago
Iteration: --- → 42.3 - Aug 10
Rank: 25
Priority: -- → P2
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

3 years ago
Iteration: 42.3 - Aug 10 → ---
(Assignee)

Comment 4

3 years ago
Created attachment 8653204 [details] [diff] [review]
Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI

Loop, removing references to JQuery
Attachment #8653204 - Flags: review?(andrei.br92)
(Assignee)

Comment 5

3 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

3 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 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

3 years ago
Created attachment 8653671 [details] [diff] [review]
Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI

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

3 years ago
Created attachment 8653674 [details] [diff] [review]
Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI

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

3 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

3 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

3 years ago
Created attachment 8654273 [details] [diff] [review]
Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI

Loop, removing references to JQuery
Attachment #8653674 - Attachment is obsolete: true
Attachment #8654273 - Flags: review?(andrei.br92)
(Assignee)

Comment 13

3 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 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+
Why are we still keeping jQuery around?
Flags: needinfo?(standard8)
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

3 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)
Great! Dave, could you follow up on that?
Flags: needinfo?(david.critchley)
(Assignee)

Comment 19

3 years ago
Created attachment 8655626 [details] [diff] [review]
Attachment to Bug 1186396 - Drop Loop's use of jQuery from the standalone UI

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

3 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

3 years ago
Flags: needinfo?(david.critchley)
(Reporter)

Updated

3 years ago
Iteration: --- → 43.2 - Sep 7
Flags: qe-verify-
https://hg.mozilla.org/mozilla-central/rev/b9c136a827c5
Status: NEW → RESOLVED
Last Resolved: 3 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.