the architecture of the pages

Coordinator
Aug 10, 2012 at 8:48 PM
Edited Aug 10, 2012 at 8:58 PM

This discussion relates to the code as it stands here: http://hilojs.codeplex.com/SourceControl/changeset/view/13593c579fb6#Hilo%2fHilo%2fhub%2fhub.js

Currently, we have the following parts:

  • page control (hub.js)
  • page controller (hubViewController.js)
  • list view controller (listViewController.js)
  • appbar controller (controls\ImageNav\imageNavController.js)

Our goals here is to encourage maintainable code. That means code that is easy to understand, to navigate, and to extend. In addition, we want to be able to test the code (as much as is reasonable).

What do you think about the current architecture? Is it easy to follow? Is it over-engineered? What do you like and what would you do differently?

 

 

Aug 13, 2012 at 5:59 PM

On first read through, the current architecture seems like overkill.

The page control is itself a controller, and knows about the appbar and the list view. Why do you need the separate controllers for individual controls? Just let the page control be the central mediator between the two.

Is the interaction between these two controls reused elsewhere? If so, I can see abstracting it out, otherwise it seems too complicated for what you're currently doing.

Aug 13, 2012 at 10:42 PM

Pardon the brain dump on this... there's a lot of design decisions and experience that went in to the decisions I made that put us in the place we're at right now. If anyone is interested, I'd love to talk about these ideas further and walk through each of the problems I'm outlining, in code. I'm sure there are details and nuances that I'm leaving out, and much more that could be discussed. I'm also interested in finding a way to get away from having a controller (mediator object) separate from page control. But as of yet, I haven't found a way to do that that satisfies the need to write fast tests.

...

I agree in principle that we should just use the page control's code. But in practice that seems to make it difficult to test and nearly impossible to compose the functionality the way I want. 

The problem largely comes down to the way WinJS pages are view-first. That is when we navigate to a page, we request the page by URI or token that represents the URI, based on the way "navigation.js" works. Because of this, we are never given direct access to the code behind (page control) of the page that we are navigating to. If we wanted to write automated tests that run against the actual rendered HTML with all the real configuration and system in place, then this is fine. But that precludes a lot of valuable testing that can be done with unit tests.

The work around this, I tried to use WinJS.UI.Pages.get to get the page control. This works to an extent. If I have a very simple page control I can call this method and get a constructor function for the type that is registered at the URI I requested. But this quickly runs in to problems, both from a functional perspective and an ideology perspective.

Personally, I hold a policy of "don't test things you don't own". Which basically says not to test frameworks and libraries that are provided to me by someone else - in this case WinRT and WinJS. By calling Pages.get, I'm getting back a constructor function that I don't own. I provided a simple object literal to Pages.define, and it does stuff to that object that I don't control. Therefore I no longer own it, and I don't want to touch it. Now I could probably be convinced that Pages.define doesn't do anything that I need to worry about, but that would require me to know what it actually does behind the scenes and this isn't something that I think we can expect of people. It's an internal framework piece that is a blackbox and should be treated that way, IMO. It facilitates a particular part of the framework, and I use it to set up page control objects. There may be a few options for making the object literal that I defined available to unit tests, but this breaks encapsulation IMO, just to facilitate testing - a code smell in itself.

Functionally, there's a bigger problem than my ideology about unit testing. Since I can't provide constructor function parameters to the Pages.get, the page control is limited to using the actual configuration of the runtime system. In some cases this isn't a big deal. In our case, this can be really bad. We are writing code to query the pictures library for pictures, and load them. If I try to write tests for a page control object that needs to query the pictures library, then I have to guarantee that a certain set of pictures is available in the pictures library before I run my tests. This isn't going to fly, at all, because we are shipping these tests with the expectation that other developers around the world will run them. Requiring devs around the world to add a certain set of images to their picture library just to run our tests isn't going to happen.

Additionally, I don't want to write tests that hit the real file system, all the time. Sometimes I don't care that the file system was touched. Sometimes I only care about the interactions between the controller in question and the object that does the query. So I want to provide a mock or stub object in the tests, in place of the real query objects. If I can't provide constructor parameters to the page control, I can't do this very easily. The options for making it work are ugly and difficult workarounds that I would prefer to avoid (setter injection, for example). 

There are some re-usable controls that we have built in to our app, as well. We're facilitating things like the common app bar implementation through page controls that are used as HTML controls. My page control for the app bar has a controller object that triggers certain events at certain times, and needs to have certain methods called at certain times. All of this is done so that the application as a whole can be composed from the smaller pieces, some of which are re-used through the app, and some of which are not. But allowing this composition of objects through a mediator is important to reduce the complexity of each individual piece, allowing the composition of these pieces through a mediator object.

The mediator object is what should be the page control. But because of the issues already mentioned, I can't test that. So I built a page controller per page, as my mediator object. The mediator takes dependencies of each controller from the page's controls, and coordinates everything for the given page.

This is basically how I build scalable JavaScript apps, no matter the runtime and framework I'm using. It scales well, and provides enough flexibility that we can make decisions on when we do / don't need a particular abstraction in a particular part of the app.

So while this app in particular could get away with much fewer classes, the trade-offs are not worth it to me. We would be reducing the number of objects, but trading smaller number of files for code that is more tightly coupled - too many concerns in one object, a higher likelihood of duplicated code, and less testable code at that. 

Coordinator
Aug 14, 2012 at 8:52 PM

Let me see if I can sum up @derickbailey's argument:

  • Page controls are difficult to test because of the expectations in navigation.js (actually, PageControlNavigator.js).
  • Minimizing the logic in a page control, by delegating to sub-controllers, simplifies the test story
  • Combining the mediator and the page control would be desirable, if it did not complicate the test story

Also, I think I see two areas of coupling.

The first is due to the page control's relationship with PageControlNavigator.js. In our tests, we observe the side effects of the interaction between navigation.js and the page control, that is, we observe changes to the DOM.

The second is the interaction between the appbar and the listview on the hub page.

Given this, I think that we may be able to both simplify the code (merge the mediator and the page control) while maintaining @derickbailey's goal by modifying PageControlNavigator.js. (For those who may not know, navigation.js is a file provided by Visual Studio in new projects, it is not part of WinJS itself.)

Thoughts?

Coordinator
Aug 14, 2012 at 8:55 PM
Edited Aug 14, 2012 at 8:58 PM

I should clarify. We can make the page control (hub.js) something that we new up just like the current controllers.

The end of hub.js, might look like: 

    WinJS.UI.Pages.define('/Hilo/hub/hub.html', new Hilo.hub.page());

Ooo, in fact, there is no reason that the call to define has to be in the hub.js file at all. It's almost like route registration anyway.

Aug 16, 2012 at 6:36 PM

That would work. The only thing you'd have to worry about is that the code that's added by the call to WinJS.UI.Pages.define is the part that loads the HTML fragment and calls the lifecycle. So in your test you'll have to manually call ready() for example, and if the code there depends on the presence of the loaded DOM elements then you'll have to supply those somehow too.

Not insurmountable, or necessarily a problem, but something to be aware of.

 

Aug 16, 2012 at 6:40 PM

Another potential option: make the actual page controls public:

var HubPageControl = WinJS.UI.Pages.define('/Hilo/hub/hub.html', { ... } );
WinJS.Namespace.define('Hilo.pages', { HubPageControl: HubPageControl });

From that point on, you can directly do new HubPageControl(element, options) and do whatever you want.

Coordinator
Aug 20, 2012 at 9:33 PM

I began experimenting with the above approach here:
http://hilojs.codeplex.com/SourceControl/changeset/view/d00bbccf0125#Hilo%2fHilo%2fmonth%2fmonth.js

Derick and I have been debating it. I'll attempt to restate Derick's arguments against it.

  • It makes dependencies implicit (the more explicit approach would be to use a constructor)
  • The tests require us to know too much about the internals of the SUT. (We have to monkey with the `ready` function because it's invoked by the constructor and has DOM manipulation it)
  • It exposes internal of the SUT.

The corresponding test code is here:
http://hilojs.codeplex.com/SourceControl/changeset/view/d00bbccf0125#Hilo.Specifications%2fspecs%2fmonth%2fmonth.spec.js

(Note that the test is actually broken because I committed the wrong thing. The `ready` is being reset after it's invoked in the constructor which accomplishes nothing. I hacked later it to be reset it on the prototype before the constructor is invoked.)

I don't share Derick's concern about the dependencies being too implicit. (I'm open to being convinced otherwise). Though I don't like having

this.queryBuilder = new Hilo.ImageQueryBuilder();

in the `ready` function.

Opine!

Coordinator
Aug 21, 2012 at 7:59 PM

A related discussion is here: https://gist.github.com/3408167

Sep 19, 2012 at 1:44 PM

For me am now learning so everything looks  find to me.

Thank you.