QubitProducts/cherrytree

Expose MemoryLocation (and BrowserLocation too) again

nathanboktae opened this issue · 7 comments

In b32bc00 MemoryLocation and HistoryLocation (later renamed to BrowserLocation which I like that rename) were no longer exposed. In tests for my app, I would setup the location before hand. I can still do that by passing in the initial location, but before it was the same API to change location before and after the app started, now it's different, rather than being the same API. I ended up creating a dummy object like this:

  var startLocation
  scenario.location = {
    setURL: function(l) {
      startLocation = l
    },
    getURL: function() {
      return startLocation
    }
  }

  //  .... various config from tests, often by a setup function callback

  // start new instance of app and get it's router
  var app = Scenario.app()
  scenario.router = app.router

  // app normally calls `listen` but not under test
  scenario.router.listen(startLocation)
  // replace dummy location with real MemoryLocation
  scenario.location = app.router.location  

Also it's nice to have these exposed to let people extend them for any reason.

Another option is have router.location available right away and not just after listen()

Do these help?

a) You can change location before calling listen by changing the value of router.options.location:

router.options.location = new BrowserLocation({
. I guess this is not very obvious as well as undocumented. But you could always have a function that creates a router createRouter({location}) for your tests.

b) You can still require in base location implementations via require('cherrytree/locations/browser') and require('cherrytree/locations/memory') if you want to customize.

a) That helps a bit, but It's still a different API
b) That only works if you're using Harmony modules

I'm not suggesting to change the initialization of cherrytree, just have cherrytree.MemoryLocation available.

I guess if you're using the standalone build, those are no longer exposed. Yeah, we could readd the cherrytree.MemoryLocation and cherrytree.BrowserLocation.

Regarding your first issue, I'm not fully following what that code snippet is demonstrating and why the API change made it worse for you? Could you show me a before/after example?

The reasoning for the API change was to make all options be passed to the cherrytree constructor function as well as making switching between 'memory' and 'browser' locations easier via an option to the constructor. And yeah, it's a breaking change, but one I wanted to add in before 2.0.0.

Previous code

scenario.location = new Scenario.cherrytree.MemoryLocation()

// start new instance of app and get it's router
  var app = Scenario.app()
  scenario.router = app.router

// configure code calls location.setURL() as normal.
scenario.router.listen(scenario.location)

Moreover I had sugar helper methods around setURL like goToFooPage. Those methods can called before and after test setup, and rather spread alot of if blocks around I just made a dummy wrapper like I had before. Pretty ugly.

I've address the cherrytree.MemoryLocation issue in #120.

Regarding the other issue however, my current thinking is now that it just so happens in your tests that location is the only thing you want to change in your tests after calling Scenario.app(), but really it's only 1 of several options to the router. What if you wanted to change or stub the behaviour of qs, logger or interceptLinks options, for example. In that case you would have to refactor how the tests are done anyway.

tl;dr - location is just one of several options to the router, I made this change (moving location to options) to make all options consistent.

Could something like this not work for your tests:

scenario.location = new Scenario.cherrytree.MemoryLocation()

var app = Scenario.app({
  router: {
    location: scenario.location
  }
})
scenario.router = app.router

Or if those are done in different parts of the test:

scenario.location = new Scenario.cherrytree.MemoryLocation()

var app = Scenario.app()

// sets scenario.router and also sets up app.router
scenario.router = app.initRouter({location: scenario.location })

Closing now as this has been partially addressed. The .listen() issue is a bit more contentious, but I do think making listen a method that just starts the routing makes for an overall simpler design..

Exposing the two location classes fully addresses this for me (IIRC, its been a bit). I dont think i was arguing for a change to listen... I can set the location before hand. Location changing is not an option of cherrytree - its a fundamental property of the browser. (I take a black box approach to testing, mocking only inputs to the whole as much as possible, which is just mainly browser location and ajax responses)