All users were logged out of Bugzilla on October 13th, 2018

Upgrade most of Loop's libraries to the latest versions

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

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

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

Its about time we upgraded to the latest versions of the libraries we use to keep pace with features and fixes etc.

I've got a couple of patches to bring these up to date. I'm keeping React & eslint separate as they will be more complicated to update.
Flags: qe-verify-
Flags: firefox-backlog+
Created attachment 8625376 [details] [diff] [review]
Upgrade Loop's test libraries.

Simple upgrade of the test libraries.
Attachment #8625376 - Flags: review?(mdeboer)
Created attachment 8625377 [details] [diff] [review]
Upgrade Loop's libs to latest versions and add some more pre-minified ones for release.

This upgrades most of our main libraries.

I've included minified versions of backbone & lodash. Its unclear if it'll help desktop perf (react "prod" is an improvement on the dev one, as they don't do extra desktop checks), but getting minified versions of these into the tree for when we sort out loop-client seems like a good idea.

I had to skip one test as I think Backbone has changed how it does things, and as a result the test fails. However, closer inspection of the test reveals that its currently testing the wrong thing, and that it seems the test is working currently by side-effects. Given that, plus the test is in the code which we're hopefully going to dump as soon as we get rid of the call-url stuff - then I think skipping is reasonable.
Attachment #8625377 - Flags: review?(mdeboer)
Comment on attachment 8625376 [details] [diff] [review]
Upgrade Loop's test libraries.

Review of attachment 8625376 [details] [diff] [review]:
-----------------------------------------------------------------

Don't mind who reviews this first.
Attachment #8625376 - Flags: review?(dmose)
Attachment #8625377 - Flags: review?(dmose)
Blocks: 1178270
Comment on attachment 8625376 [details] [diff] [review]
Upgrade Loop's test libraries.

Review of attachment 8625376 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; r=dmose.

Please post to loop-services-dev or some other appropriate place once this has landed, with the notes to the release notes, and be sure to call out the one change that people are likely to care about: the fact that sinon now makes it possible to stub getters and setters with a documentation link to how to do that.
Attachment #8625376 - Flags: review?(mdeboer)
Attachment #8625376 - Flags: review?(dmose)
Attachment #8625376 - Flags: review+
Comment on attachment 8625377 [details] [diff] [review]
Upgrade Loop's libs to latest versions and add some more pre-minified ones for release.

Review of attachment 8625377 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit concerned about switching the desktop stuff to the minified versions:

* it's not clear that it'll make a performance difference, given that there's no network load, and this stuff is all compressed inside a JAR anyway
* even if the source-mapping stuff works, the console doesn't currently support source maps (though I think many of the other devtools do)

meaning that it might make debugging production nightly builds a bit more painful.

That said, I'm not too worried about this, so I'll leave it up to you.

It also looks like this patch will need a rebase before landing.

Please post to loop-services-dev with links to the changelog once this patch lands.

::: browser/components/loop/test/shared/views_test.js
@@ +460,5 @@
>        });
>  
>        describe("#startPublishing", function() {
>          beforeEach(function() {
> +          sandbox.stub(fakePublisher, "listenTo");

Is this the right change, now that the test has been removed?

@@ +471,5 @@
>            sinon.assert.calledOnce(fakeSession.publish);
>          });
>  
> +        // XXX This test would need reworking, but the code should be going
> +        // away soon.

Instead of soon, how about either "as part of" or "after" bug XXX (the call-URL removal bug) lands?
Attachment #8625377 - Flags: review?(mdeboer)
Attachment #8625377 - Flags: review?(dmose)
Attachment #8625377 - Flags: review+
Keywords: leave-open

Updated

3 years ago
Iteration: 41.3 - Jun 29 → 42.1 - Jul 13
(In reply to Pulsebot from comment #9)
> https://hg.mozilla.org/integration/fx-team/rev/886572f8cb41

Landed this second patch without the minified for production as in my testing, we were using about 200k more of memory.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/886572f8cb41
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.