Closed Bug 1186396 Opened 6 years ago Closed 6 years ago

Drop Loop's use of jQuery from the standalone UI

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
3

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.2 - Sep 7
Tracking Status
firefox43 --- fixed
Blocking Flags:
backlog tech-debt

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)

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
Points: --- → 3
User Story: (updated)
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)
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)
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.
Iteration: 42.3 - Aug 10 → ---
Loop, removing references to JQuery
Attachment #8653204 - Flags: review?(andrei.br92)
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)
(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-
Loop, removing references to JQuery
Attachment #8653204 - Attachment is obsolete: true
Attachment #8653671 - Flags: review?(standard8)
Attachment #8653671 - Flags: review?(andrei.br92)
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)
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-
(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.
Loop, removing references to JQuery
Attachment #8653674 - Attachment is obsolete: true
Attachment #8654273 - Flags: review?(andrei.br92)
(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?
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)
Loop, removing references to JQuery
Attachment #8654273 - Attachment is obsolete: true
Attachment #8655626 - Flags: review?(standard8)
Attachment #8655626 - Flags: review?(andrei.br92)
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+
Flags: needinfo?(david.critchley)
Iteration: --- → 43.2 - Sep 7
Flags: qe-verify-
https://hg.mozilla.org/mozilla-central/rev/b9c136a827c5
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.