geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

saving save method and running fails #222

Closed ivanseidel closed 10 years ago

ivanseidel commented 10 years ago

I was starting to use Geddy and async library, to enable me to do parallel stuff.

However, the code I thought first was not working:

// Save in parallel
async.parallel({
    myEvent: geddy.model.Event.create({}).save,
    myUser: geddy.model.User.create({}).save
}, function (err, saved){
    console.log(saved.myEvent, saved.myUser);
});

And it lead me to simplify it. And the same error occurs with this code:

var evt = geddy.model.Event.create({});
var saveMethod = evt.save;
saveMethod(function (err, model){
    console.log(err, model);
});

The generated error is:

TypeError: Object #<Object> has no method '_commitAssociationChanges'
at save (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:115:14)
at Context.<anonymous> (/<myProject>/test/models/tag.js:74:3)
at Test.Runnable.run (/<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runnable.js:216:15)
at Runner.runTest (/<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:373:10)
at /<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:451:12
at next (/<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:298:14)
at /<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:308:7
at next (/<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:246:23)
at Object._onImmediate (/<myProject>/node_modules/jake-mocha/node_modules/mocha/lib/runner.js:275:5)
at processImmediate [as _immediateCallback] (timers.js:330:15)

Any recommendations? Is this some sort of bug? Is there any reason why I shouldn't be doing this?

danfinlay commented 10 years ago

This is a binding issue. You pass the function, but those functions require their original context. Try this instead:

// Save in parallel
var myEvent = geddy.model.Event.create({});
var myUser = geddy.model.User.create({});

async.parallel({
    myEvent: myEvent.save.bind( myEvent ),
    myUser: myUser.save.bind( myUser )
}, function (err, saved){
    console.log(saved.myEvent, saved.myUser);
});
ivanseidel commented 10 years ago

I see @flyswatter ... I was taking a deep look at the code. Isn't there a way to fix this, so that we don't need to use the binding stuff? Or this is really, a specific case of use...?

Also, isn't there a (prettier) way of doing that workaround? I mean, without using helper variables...

Thanks for the answer anyway!

danfinlay commented 10 years ago

This is pretty much a Javascript issue, not a Geddy issue. Needing to pass a context with a function when asking a module to do some work for you is a pretty common pattern. Even the async documentation has a "common pitfalls" section that discusses exactly this issue.

Rather than fight against it, I recommend learning why this is.

Passing a function only passes the function. But functions will also refer to properties of their context (calls to this). If you only pass a function to a module, the meaning of this becomes that module, and now there's really no way for it to know what it was supposed to do. You have to pass it a reference to the parent object somehow.

Bind is a wonderful helper function that comes in the core of Javascript, designed to simplify exactly this kind of scenario. You call it on a function, give it a context, and any number of arguments to lock to the function, and it gives you back a function that can be called without those arguments, and with the context assumed.

That means you can also do things like this:

var User = geddy.model.User;
var Event = geddy.model.Event;

async.parallel([
  User.first.bind(User, {name:'Bob'}),
  Event.first.bind(Event, {price:{le:50}})
], function (err, results){
   // Enjoy the benefits.
});

If you're really concerned about being as terse as possible you might like the module ap by substack which is basically bind except you don't have to set the context, it just grabs the context of the function object that you pass it.

danfinlay commented 10 years ago

A sample usage of ap:

var bind = require('ap').curry;
var User = geddy.model.User;
var Event = geddy.model.Event;

async.parallel([
  bind(User.first, {name:'Bob'}),
  bind(Event.first, {price:{le:50}})
], function (err, results){
   // Enjoy the benefits.
});
ivanseidel commented 10 years ago

Got it!

I usually write methods like this:

var self = this;
this.myMethod = function(){
  [...]
  // When I need to use the context, I use the self helper var
  self.save(...);
}

Is it bad, or something like that? (A little more than what I asked...)

danfinlay commented 10 years ago

No, this is a very common pattern, people use self, that, _this, all sorts of awful looking things, but it's an easy fix to the issue of changing scope between callbacks, and once you understand the pattern it's really pretty painless.

Some people will use bind with a closure to preserve the calling context instead. That technique makes this:

obj.doSomething = function() {
  var that = this;
  setTimeout(function() {
    // this is the window
    // that is the obj
    that.doSomethingElse();
  }, 50);
};

look like this:

obj.doSomething = function() {
  setTimeout((function() {
    // this is the obj
    this.doSomethingElse();
  }).bind(this), 50);
};
ivanseidel commented 10 years ago

I understand the pattern, just wasn't aware of the no-pattern way... Thanks, It solved it

danfinlay commented 10 years ago

Cool, glad I could help!

ivanseidel commented 10 years ago

Another problem that might be the same thing!

        var event = geddy.model.Event.create({});
        var user = geddy.model.User.create({});

        // Save in parallel
        async.parallel({
            event: event.save.bind(event),
            user: user.save.bind(user),
            tag: function (cb){
                Tag.createUnique({ value: 33 }, cb);
            },
        }, function (err, saved){
            if(err) { return done(err) }; 

            var tag = saved.tag;
            tag.setEvent(saved.event);
            tag.setUser(saved.user);

            tag.save(function(err, model){
                if(err) { return done(err) };

                console.log(err);
                console.log(tag);
                var evt = tag.getEvent(); <-- Problem occurs here
                done();
            });

        });

Is outputting:

 TypeError: object is not a function
      at utils.mixin.load (/<myProject>/node_modules/geddy/node_modules/model/lib/adapters/memory/index.js:73:9)
      at Object.obj.all (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:395:25)
      at Function.obj.first (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:357:18)
      at association._getAssociation (/<myProject>/node_modules/geddy/node_modules/model/lib/association/index.js:170:34)
      at null.getEvent (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:249:44)
      at /<myProject>/test/models/tag.js:93:21
      at /<myProject>/node_modules/geddy/node_modules/model/lib/index.js:523:9
      at utils.mixin.update (/<myProject>/node_modules/geddy/node_modules/model/lib/adapters/memory/index.js:175:7)
      at Object.obj.update (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:512:27)
      at Function.obj.save (/<myProject>/node_modules/geddy/node_modules/model/lib/index.js:452:29)

I guess that the tag object is not binded... right @flyswatter ?

danfinlay commented 10 years ago

No, that pattern works fine. You could even:

 async.parallel({
            event: function(cb){event.save(cb);},,
            user: function(cb){user.save(cb);},

If you wanted to, and before I knew how to use bind I did that a few times.

My guess is Tag.createUnique is an object not a function.

ivanseidel commented 10 years ago

The line error is after it. tag is logging correcly:

{ type: 'Tag',
  _saved: true,
  isValid: [Function],
  save: [Function],
  updateProperties: [Function],
  updateAttributes: [Function],
  toJSON: [Function],
  toData: [Function],
  toObj: [Function],
  toString: [Function],
  _getAssociation: [Function],
  _createAssociation: [Function],
  _removeAssociation: [Function],
  _commitAssociationChanges: [Function],
  clone: [Function],
  getPhotos: [Function],
  addPhoto: [Function],
  removePhoto: [Function],
  getUser: [Function],
  setUser: [Function],
  removeUser: [Function],
  getEvent: [Function],
  setEvent: [Function],
  removeEvent: [Function],
  _events: {},
  createdAt: Sat Aug 23 2014 01:40:46 GMT-0300 (BRT),
  updatedAt: Sat Aug 23 2014 01:40:46 GMT-0300 (BRT),
  value: 33,
  name: 'fio estreito',
  facebookGallery: undefined,
  userId: 'DA64C7E0-9616-4C03-B64B-7470550C47C3',
  eventId: '61C83C06-EFA1-473C-8BCD-6EAFBBA1B861',
  id: '41C866D4-B2F6-4E9A-BDB0-04C1F261E916',
  _unsavedAssociations: [] }
mde commented 10 years ago

Excellent work explaining this stuff, @flyswatter!

ivanseidel commented 10 years ago

Oh... solved it. Dumb stuff.... (I guess it's time to go to sleep).

get[Association] methods are async... thought they were loaded on fetch. Problem number 2 fixed.

Thanks @flyswatter egain!

ivanseidel commented 10 years ago

By the way, @mde , GREAT work on GeddyJs! I was almost killing myself with %!#@# Sails, when discovered Geddy...

Such a well implemented, generic and documented Framework! Congratulations.

danfinlay commented 10 years ago

Thanks, @mde :)

Hey @ivanseidel, you will probably enjoy @ben-ng's latest work with eager-loading associations, if you know what associated models you want. You could say:

geddy.model.User.first({id:4}, {includes:{'Tag':{'Event':'Photo'}}}, function (err, user){
  if (err) throw err;

  console.log("Wow, a relationship 3 levels deep!:");
  console.log( user.tags[1].event.photos[1] );
ivanseidel commented 10 years ago

console.log("Wow, a relationship 2 levels deep!:");

Is it one query? Or associated in code @flyswatter ?

danfinlay commented 10 years ago

It makes two calls to the database, but the first one is a lean, ID-fetching query.

ivanseidel commented 10 years ago

Awesome! Is it going to be released in the next version?

danfinlay commented 10 years ago

What if I told you it's already live?

ivanseidel commented 10 years ago

lol.

Those kind of stuff are always under development in Sails.js. I think I'm used to it...

danfinlay commented 10 years ago

No seriously, it's in the current version.

ivanseidel commented 10 years ago

I believe you @flyswatter , it's just that this wouldn't be true with Sails.

(Yes, I kind of 'dislike' Sails, because it teached me how to do a non-generic framework, in such a way that you can't explain what is wrong with it, but at the same time, feels obligated to learn and use it).

danfinlay commented 10 years ago

Oh I've gotcha.

I was never going to use Sails when I learned it was built on Express, since it obscures the streaming http request object, which is one of the coolest things node provides!

ivanseidel commented 10 years ago

that's the word: obscures... The docs I was reading, was the code itself... Documentation was missing, code was not organised, and I was copying and pasting code (blah.)

But I will use MongoDB... So includes will not work properly... =/

But ok... everything is fine now! =)