Drop Loop's use of Backbone - replace with an Events library or custom Event management

RESOLVED INCOMPLETE

Status

P2
normal
Rank:
29
RESOLVED INCOMPLETE
3 years ago
2 years ago

People

(Reporter: standard8, Unassigned)

Tracking

({memory-footprint, perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

(Reporter)

Description

3 years ago
We used backbone as an MVC tool in the early days of Hello. Since then we've moved away from using Backbone for views and models, and now it is only used for Events.

Since we don't need most of the functionality, we should drop it, and use something specifically for handling the events (or write our own).
(Reporter)

Updated

3 years ago
Rank: 29

Comment 1

3 years ago
(In reply to Mark Banner (:standard8) from comment #0)
> We used backbone as an MVC tool in the early days of Hello. Since then we've
> moved away from using Backbone for views and models, and now it is only used
> for Events.
> 
> Since we don't need most of the functionality, we should drop it, and use
> something specifically for handling the events (or write our own).

I can work on this, please tell me if you will be available to help me out a bit if needed though.

Thanks a lot!
Flags: needinfo?(standard8)
(Reporter)

Comment 2

3 years ago
Hi Pablo, thanks for the interest.

I think there's several parts to this bug, and I think we'll want to split it out into multiple bugs. You're welcome to work on none, some or all of them ;-) . Here's what I think needs doing:

1) Rewrite standaloneAppStore.js and conversationAppStore.js to use the createStore from store.js. See textChatStore.js for an example.

2) Remove the warn,warnL10,error,errorL10,success,successL10n functions from NotificationCollection in models.js as they aren't used.

3) In models.js we need to rewrite the NotificationModel/NotificationCollection to become a store. With the removed functions from the previous step, this should be a simplish store. views.jsx will need NotificationListView updating to use the new store.

We're using a flux methodology, here's some background info to that: https://blog.mozilla.org/standard8/2015/02/09/firefox-hello-desktop-behind-the-scenes-flux-and-react/

4) Find a replacement library for event handling to replace Backbones. We mainly only use on/off and trigger("change"), so we should be able to go with something pretty simple.

At the moment I think https://github.com/chrisdavies/eev is the simplest/best one I've seen that has tests and is being maintained, but there may be more suggestions out there.

5) Replace remaining instances of Backbone.Events with the new event emitter. We might be able to do some of this piecemeal, but with the previous steps complete, I think it'll be pretty much removing all the remaining instances.

6) (if not done as part of 5) Remove backbone!

I think we want to split out 1 to 3 to separate bugs. Item 2 is a nince small one to get started on - it needs the functions removing and tests updating.

4 to 6 can be done in this one bug, using multiple patches if necessary.

Let me know what you think. Feel free to start filing bugs/working on things.
Flags: needinfo?(standard8)

Comment 3

3 years ago
(In reply to Mark Banner (:standard8) from comment #2)
> Hi Pablo, thanks for the interest.
> 
> I think there's several parts to this bug, and I think we'll want to split
> it out into multiple bugs. You're welcome to work on none, some or all of
> them ;-) . Here's what I think needs doing:
> 
> 1) Rewrite standaloneAppStore.js and conversationAppStore.js to use the
> createStore from store.js. See textChatStore.js for an example.
> 
> 2) Remove the warn,warnL10,error,errorL10,success,successL10n functions from
> NotificationCollection in models.js as they aren't used.
> 
> 3) In models.js we need to rewrite the
> NotificationModel/NotificationCollection to become a store. With the removed
> functions from the previous step, this should be a simplish store. views.jsx
> will need NotificationListView updating to use the new store.
> 
> We're using a flux methodology, here's some background info to that:
> https://blog.mozilla.org/standard8/2015/02/09/firefox-hello-desktop-behind-
> the-scenes-flux-and-react/
> 
> 4) Find a replacement library for event handling to replace Backbones. We
> mainly only use on/off and trigger("change"), so we should be able to go
> with something pretty simple.
> 
> At the moment I think https://github.com/chrisdavies/eev is the
> simplest/best one I've seen that has tests and is being maintained, but
> there may be more suggestions out there.
> 
> 5) Replace remaining instances of Backbone.Events with the new event
> emitter. We might be able to do some of this piecemeal, but with the
> previous steps complete, I think it'll be pretty much removing all the
> remaining instances.
> 
> 6) (if not done as part of 5) Remove backbone!
> 
> I think we want to split out 1 to 3 to separate bugs. Item 2 is a nince
> small one to get started on - it needs the functions removing and tests
> updating.
> 
> 4 to 6 can be done in this one bug, using multiple patches if necessary.
> 
> Let me know what you think. Feel free to start filing bugs/working on things.

Definitely looking forward to help out with what I can :)

I'm not sure about filing them myself though since I wouldn't know all the details, but if you don't mind please link me the bugs when someone does so I can start submitting fixes.
Flags: needinfo?(standard8)
(Reporter)

Updated

3 years ago
Depends on: 1225035
(Reporter)

Updated

3 years ago
Depends on: 1225038
(Reporter)

Updated

3 years ago
Depends on: 1225057
(Reporter)

Comment 4

3 years ago
Thanks Pablo, I've filed some bugs and assigned them to you for now - see the dependency list for this bug. If you decide not to take them, just un-assign yourself. I'll tentatively assign you to this one for now as you're working on the collection, and it'll mean other folks know that you're working in this area.
Assignee: nobody → pablo_lm1
Flags: needinfo?(standard8)
Whiteboard: [tech-debt][perf] → [tech-debt]
(Reporter)

Updated

3 years ago
Assignee: pablo_lm1 → nobody
(Reporter)

Comment 5

2 years ago
Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.