Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transformed documents are non-reactive #667

Closed
m13t opened this issue May 21, 2014 · 12 comments
Closed

Transformed documents are non-reactive #667

m13t opened this issue May 21, 2014 · 12 comments

Comments

@m13t
Copy link

m13t commented May 21, 2014

I've been working on a very basic (work in progress) Model class for a project I am working on which seems to work fine with Iron Router, but I'm having a bit of an issue with reactivity once the model is updated.

For example, when declaring a route with my data function returning a Model using findOne on my collection, the data source works fine and everything is displayed on screen as expected, until that is, I call save on my model after updating some properties. At this point, there is no reactive update. However, if I call the same methods to update the model from the console, everything updates.

model.js

var createModel = function(collectionName)
{
    var collection = new Meteor.Collection(collectionName,
    {
        transform: function(doc)
        {
            return new Base(doc);
        }
    });

    var Base = function(doc)
    {
        _.extend(this, doc);
    }

    _.extend(Base.prototype,
    {
        constructor: Base,

        isNew: function()
        {
            return !_.has(this, '_id');
        },

        save: function(callback)
        {
            var self = this;
            var keys = _.keys(self);
            var obj = {};

            _.each(keys, function(a)
            {
                obj[a] = self[a];
            });

            if (self.isNew())
            {
                // Save New
                return collection.insert.call(collection, obj, callback);
            }
            else
            {
                // Update Old
                return collection.update.call(collection, { _id: self._id }, { $set: _.omit(obj, '_id') }, callback);
            }
        },

        remove: function(callback)
        {
            return collection.remove.call(collection, { _id: this._id }, callback);
        },

        toString: function()
        {
            return 'Model(' + collection._name + ')';
        }
    });

    _.extend(Base,
    {
        extend: function(protoProps)
        {
            var self = this;

            _.extend(self.prototype, protoProps);

            return self;
        },

        toString: function()
        {
            return 'Model(' + collection._name + ')';
        }
    });

    _.each([ 'allow', 'deny', 'find', 'findOne', 'insert', 'remove', 'update', 'upsert' ], function(a)
    {
        Base[a] = function()
        {
            return collection[a].apply(collection, arguments);
        }
    });

    return Base;
}

and in my routes setup I have the following

this.route('wikiPage',
{
    path: '/wiki/:slug',
    template: 'wiki',

    onBeforeAction: 'dataNotFound',

    waitOn: function()
    {
        return Meteor.subscribe('wiki');
    },

    data: function()
    {
        var params = this.params;
        return Collections.Wiki.findOne({ slug: params.slug });
    }
});

Collections.Wiki is essentially returned from createModel as above. So the data returned from findOne is an instance of the model. Now somewhere in my template event handlers I update a property, call save and nothing happens unless I do a manual refresh. Whereas when using the same code in the JS console of the browser, it updates fine.

Has anyone else come across this issue at all?

@m13t
Copy link
Author

m13t commented May 22, 2014

I have created a project that reproduces / better explains the issue I am having.

https://github.com/lewislambert/Iron-Router-Issue-667

@tmeasday tmeasday added this to the Next Release milestone Jun 12, 2014
@tmeasday tmeasday self-assigned this Jun 12, 2014
@tmeasday
Copy link
Contributor

Hey @lewislambert - thanks for the reproduction. I am seeing the issue you've outlined and it's not immediately obvious why.

I'll have to trace through to try and figure out what's going on.

@tmeasday
Copy link
Contributor

OK, I worked out what the problem is. The issue is kind of complicated, but I'll do my best to explain:

  1. Blaze-Layout "emboxes" the data context. This is a blaze thing that means that it'll only trigger reactivity if it changes.

  2. The way your model works means that you event handler ends up changing the existing data context to look exactly like what comes out of the db.

  3. So what happens is this:

    i. You change the in-memory data context
    ii. You save it to the db, triggering the findOne to rerun in the data function
    iii. The new data value gets sent to the embox but it appears to the embox to be exactly the same as the original.
    iv. So reactivity is quashed and your template doesn't redraw.

In the case of changing it on the commandline, we don't see this behaviour, because it's a different in-memory object that you modify and then save.

I'm not really sure what the solution to this problem is. I fear that your approach of modifying the document and then saving it will trigger this kind of problem around the place in Meteor (as UI makes extensive use of emboxing). @ryw - have you guys experienced this issue with minimongoid (which I assume does something similar to this?)

@m13t
Copy link
Author

m13t commented Jun 13, 2014

Hi. Thanks for the update and I'm glad to hear it wasn't just me doing things the 'wrong way'. I certainly had trouble figuring it out as it was frustrating that it worked from the console no problems. Now I know where the problem is, I'll try and find a workaround until the bug is fixed.

Thanks

@tmeasday
Copy link
Contributor

@lewislambert - I guess what I was trying to tell you is that I think you might find you have problems around the place with Meteor and this technique. I don't think it's "wrong" by any means, but I don't really see a "bug" in IR that is going to be "fixed" to solve this for you.

@m13t
Copy link
Author

m13t commented Jun 16, 2014

@tmeasday OK thanks for that. So assuming then that I should stick with the standard meteor collections approach, have you any ideas how I might be able to still simplify a lot of collection logic by abstracting out into Models? I've got quite a lot of collections in a project I am working on and I thought as there isn't really a good abstraction layer for Models out there (some but none really completed/fully functional) I could kill two birds with one stone. Now I'm thinking it may not be a good idea.

@tmeasday
Copy link
Contributor

@lewislambert - we use models all over the place, but we just don't go to the point of implementing a .save() function -- we call X.insert or X.update when we want to do that.

Again, not to say that there's anything wrong with that. This is why I'm intrigued about whether those who are using minimongoid have seen this kind of issue before -- @ryw @queso ?

@queso
Copy link

queso commented Jun 17, 2014

If you look at save on minimongoid, it just wraps update to have minimongo syntax like _id, {$set:}, etc. Minimongoid doesn't go so far as to implement much new, just handy wrapped up methods.

@tmeasday
Copy link
Contributor

Hmm. I would think this could lead to the same scenario -- where a record is modified "in-place", then saved to the the db, triggering emboxed EJSON.equals checks to think it's unchanged and kill reactivity.

I should make a simple reproduction of this with a {{#with}} block (which emboxes too)...

@tmeasday
Copy link
Contributor

OK. I realised it's not embox, but us. I've opened a new ticket which should make it all clearer for us. Thanks @lewislambert

@m13t
Copy link
Author

m13t commented Jun 17, 2014

Yes my model class (not in full above) is also just a wrapper for the Collections insert/update but with diffing so that only changed attributes are updated, arrays are pushed/pulled etc which is why I couldn't understand the issue. Relieved it's not just me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants