AmpersandJS / ampersand-virtual-dom-mixin

MIT License
33 stars 3 forks source link

Possible race condition somewhere #3

Closed ferdinandsalis closed 9 years ago

ferdinandsalis commented 9 years ago

I ran into an issue while using this mixin. I will try to explain as best a possible — sorry for the inadequate title :blush:.

Scenarios

  1. Navigate to the view via the router
  2. Reload the view

    Context

This is given.

initialize: function (spec) {
  this.collection.getOrFetch(spec.id, { all: true }, (err, model) => {
    if (err) {
       console.log('Couldnt find a model with id: ' + spec.id)
    } else {
      this.model = model;
    }
  this.on('change', this.render);
  });
},

The following are the two permutations (views are adapted accordingly).

render: function () {
  this.renderWithTemplate(this.model);
  return this;
}

Here both scenarios work.

render: function () {
  this.renderWithTemplate(this);
  return this;
}

Only the first one renders the page. However the reload only renders the following error: TypeError: model is undefined. I did try to step debug the error. It seems that there is some kind of race condition since relinquishing the control back to the js runtime renders the page normally.

latentflip commented 9 years ago

Hey @ferdinandsalis can you show the full code for the view that's having issues? I'm not quite sure why you have those two render functions, one passing this, the other passing this.model. Thanks!

ferdinandsalis commented 9 years ago

I guess its not clear. I don’t have two render functions. Just depending on which one it does not work in the second scenario.

ferdinandsalis commented 9 years ago

This is the permutation of the view that triggers the error.

var app = require('ampersand-app');
var AmpersandView = require('ampersand-view');
var virtualDom = require('ampersand-virtual-dom-mixin');

module.exports = AmpersandView.extend(virtualDom, {
  initialize: function (spec) {
    this.pageTitle = 'A Page';

    this.collection.getOrFetch(spec.id, { all: true }, (err, model) => {
      if (err) {
        console.log('Couldnt find a model with id: ' + spec.id)
      } else {
        this.model = model;
      }
    });

    this.on('change', this.render);
  },

  render: function () {
    this.renderWithTemplate(this);
    return this;
  },

  derived: {
    goBack: {
      fn: function () { return '/favorites/'; }
    }
  },

  template: require('../templates/show.jade'),
});
latentflip commented 9 years ago

@ferdinandsalis so my guess is that your template is trying to do something like model.foo which, given that it may be rendered before the model is actually available, is raising an exception, because this.model.foo doesn't exist.

You'll probably want to make sure your template render can handle model not being set, or default it to an empty object for the initial render.

ferdinandsalis commented 9 years ago

Here is the template.

article

  a(href=goBack) Back

  article
    h1= model.title
    p= model.description
    p= model.fullTitle
ferdinandsalis commented 9 years ago

@latentflip ok will think about that. Though not sure why it works when I pass in this.model instead. Thanks so much, anyway.

latentflip commented 9 years ago

@ferdinandsalis right, so the first time your view get's rendered, it may not have a model cos it hasn't loaded yet.

So, you have a couple of options:

article
  a(href=goBack) back

  article
    if !model
      p Loading...
    else
      h1= model.title
      p= model.description
      p= model.fullTitle

Or if you just want it blank until the data loads

article

  a(href=goBack) Back

  article
    h1= model &&model.title
    p= model && model.description
    p= model && model.fullTitle
latentflip commented 9 years ago

@ferdinandsalis it works when you pass this.model instead of this, because then your template is effectively doing templateData.title or templateData.description which is just going to return undefined, instead of templateData.model.title which is going to raise an exception.

You could also just do

module.exports = AmpersandView.extend(virtualDom, {
  initialize: function (spec) {
    this.pageTitle = 'A Page';
    this.model = {}; //some dummy model data until the real one's loaded

    this.collection.getOrFetch(spec....
ferdinandsalis commented 9 years ago

Thanks! Now I understand.

latentflip commented 9 years ago

Cool, going to close this as it's not exactly a bug in this code, but will add an issue to remind that we should explain this issue in the docs as I'm sure you won't be the first to hit it.

ferdinandsalis commented 9 years ago

The last of suggestions setting this.model to an object does not work because of the type casting, so it seems. The others work though.

ferdinandsalis commented 9 years ago

I guess one could do just this this.model = new AmpersandModel() instead.