dustinfarris / ember-redux-route-connect

Connect your routes to redux state/dispatch
MIT License
5 stars 2 forks source link

Consider calling super earlier in setupController #6

Closed foxnewsnetwork closed 7 years ago

foxnewsnetwork commented 7 years ago

TL;DR

Right now, this._super(...arguments) lives in https://github.com/dustinfarris/ember-redux-route-connect/blob/master/addon/index.js#L82 please consider moving it to https://github.com/dustinfarris/ember-redux-route-connect/blob/master/addon/index.js#L55 or abouts:

this._super(...arguments);
// Add new props to the controller
        Object.keys(props).forEach(name => {
          defineProperty(controller, name, computed(() =>
            stateToComputed(redux.getState(), this.getCurrentParams(controller))[name]
          ).property().readOnly());
        });

Why

The attribute model on Ember.Controller objects tend to be given special treatment by the ember framework, notably, it's where a route's modelFor method goes to look for a given route's controller's resolved model. This allows an Ember user to get a route's parent route's model in the model hook (or wherever) and provides a convenient way for get sensible data for example:

// router.js
router.map(function() { 
  this.route('level', { path: 'level/:id' }, function() { 
    this.route('bosses', function() { 
      this.route('new');
    });
  });
});

// routes/level.js
model({id}) {
  return findLevel(id);
}

// routes/level/bosses.js
model() { 
  const level = this.super();
  return level.get('bosses');
}

// routes/level/bosses/new.js
model() { 
  const level = this.modelFor('level');
  return level.bosses.create({});
} 

Going into the world of redux, we could keep much of this sensible route-tree-based parent model referencing if we permit ourselves to know that Ember.Route's model method calls modelFor which in turn does controller.get('model') under the hood, and declare our statesToCompute function like so:

function statesToCompute(state, params) { 
  return {
    model: myLevelRouteState(state, params)
  };
}

The Problem

However, because we now have a prop named model that is a readOnly computed property, Ember croaks with something along the lines of:

Error while processing route: testspace.scene.characters.new Cannot set read-only property "model" on object: (generated testspace.scene.characters.new controller) Error
    at ComputedProperty.computedPropertyThrowReadOnlyError [as _throwReadOnlyError] (http://localhost:4200/assets/vendor.js:26893:11)
    at ComputedProperty.computedPropertySetEntry [as set] (http://localhost:4200/assets/vendor.js:26878:12)
    at Object.set (http://localhost:4200/assets/vendor.js:31883:12)
    at Class.setupController (http://localhost:4200/assets/vendor.js:36635:21)
    at Class.setupController (http://localhost:4200/assets/vendor.js:174039:23)
    at Class.superWrapper [as setupController] (http://localhost:4200/assets/vendor.js:50196:22)
    at Class.setup (http://localhost:4200/assets/vendor.js:36243:12)
    at callHook (http://localhost:4200/assets/vendor.js:66255:36)
    at _handlerEnteredOrUpdated (http://localhost:4200/assets/vendor.js:67835:5)
    at handlerEnteredOrUpdated (http://localhost:4200/assets/vendor.js:67847:5)

because the this._super(...arguments) call is setting the model onto the controller which is suppose to be readOnly

Proposed Solution

Move the this._super(...arguments) up before we define the properties onto the controller so that the controller.set('model', model) implicit in setupController is rendered harmless.

Let me know what you think; I can PR if you're on board with this idea

Alternative Solution

Drop the super call altogether and instead write in the README instructions for including model into the statesToCompute function.

Considering that in ember code, all setupController does is set the model, see here (https://github.com/emberjs/ember.js/blob/v2.10.0/packages/ember-routing/lib/system/route.js#L1698), it can safely be dropped if we just instruct users that model must be set by themselves.

In my opinion, this "alternate solution" is actually quite in-line with how redux demands we handle remote data anyway... since, (and I could be wrong since I'm no redux expert) redux seems to want us to separate the concerns of making a remote request (e.g. dispatch some action) from actually fetching the result app-state after the action has finished

dustinfarris commented 7 years ago

Fixed in #7