Introduce Foxbox API sub-module. Maintain proper services in-memory cache in a separate submodule. Use EventDispatcher for Network, Settings and WebPush sub-modules. (fixes #116). r=gmarty #120
Conversation
| getURL: Symbol('getURL'), | ||
| }); | ||
|
|
||
| export default class API { |
There was a problem hiding this comment.
One of the ideas I'm still forming in my head is to make API work a bit like DB - it will be initialized immediately, but since all its methods are sync they will wait until user is authenticated and box is discovered....
|
I left a few comments. |
azasypkin
changed the title
| @@ -0,0 +1,195 @@ | |||
| import { waitFor } from '../../test-utils'; | |||
There was a problem hiding this comment.
Here not all cases covered, just most important and obvious. Should be good enough to start with.
There was a problem hiding this comment.
Yes, we need code coverage to know where to focus on writing new unit tests in the future.
|
I think it should be ready for review, r+ @gmarty |
| this._dispatchEvent('box-online', foxboxOnline); | ||
|
|
||
| this[p.net] = new Network(this[p.settings]); | ||
| this[p.net].on('online-state-changed', (online) => { |
There was a problem hiding this comment.
Naming of events is hard :/ Tell me if you have better names for the events!
There was a problem hiding this comment.
The more I think about it, the less I like the -changed suffix. An event means a change happened, so that sounds like a redundancy to me. Also, we should take inspiration from the browser event, in this particular case, online seems appropriate.
Likewise, in message-received, a message related event is always going to be about receiving a message, so message looks good enough to me.
I know I suggested preterite verb, but maybe name only event names are better.
Do you have any preferences on that?
There was a problem hiding this comment.
I know I suggested preterite verb, but maybe name only event names are better.
:) Yeah, I have the same feeling honestly, just followed what we agreed on previously ;) So, remove ``preterite verb` part and we're good? I'm fine with this.
Likewise, in message-received, a message related event is always going to be about receiving a message, so message looks good enough to me.
Not always :) In message app we have message-received message-sent message-failed-to-sent :) But in this case just message should be ok.
There was a problem hiding this comment.
I think we can try to not be very strict right now, and just follow "lower case + dashes to separate words + common sense" rule and see how it goes :) In some places preterite verb makes a lot of sense.
|
I did the review (except the tests), but because it's a big PR I want to make sure I take the time to fully understand the changes and what it means for the architecture of the lib. But so far so good. That's a great job! |
Sure, take your time and thanks for the review! I won't touch this PR to until you're fully done so that you have original "snapshot". |
| services = new Services(dbStub, apiStub, settingsStub); | ||
| }); | ||
|
|
||
| it('getAll returns all currently cached services', function(done) { |
There was a problem hiding this comment.
nit: you're using double quotes around method names in the api test file above.
|
OK, I left a couple of nit/question but it looks good to me. r+ |
…ache in a separate submodule. Use EventDispatcher for Network, Settings and WebPush sub-modules. (fixes #116). r=gmarty
No description provided.