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

no 'fetched' event on model when collection is loaded #241

Closed
bogus34 opened this issue Feb 13, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@bogus34
Copy link

commented Feb 13, 2014

Maybe it will be practical if 'fetched' event were fired on each model loaded in collection.
Say if I have model User and collection Users. And User has some special logic in its 'fetched' handler. When I call Users().fetch() this logic is simply skipped.

@fritx

This comment has been minimized.

Copy link

commented Feb 18, 2014

yeah, I met the same problem as yours..
I rewrited fetch method on collection to forcely fetch each model after collection fetches:

fetch: function(options) {
  options = options || {};
  var forModel = options['each'];
  return db.Collection.__super__fetch.call(this, options)
    .then(function(collection) {
      return collection.invokeThen('fetch', forModel)
        .then(function () {
            return collection;
        });
    });
}
@bendrucker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

Right now collection.fetch only fires a fetched event for the collection itself. The models are never actually being called with fetch so I'm not sure it's entirely appropriate to fire events.

@fritx your method is going to create collection.length unnecessary DB calls so I'd highly recommend against using it.

@bogus34 If you can share what you're actually doing in your fetched handler(s) I can try to help you find a way to make things work.

@bendrucker bendrucker added the question label Feb 18, 2014

@fritx

This comment has been minimized.

Copy link

commented Feb 18, 2014

yeah, thanks @bendrucker, you alarmed me..
:[ I think we had written too much code mistakenly into the model.fetch!

@fritx

This comment has been minimized.

Copy link

commented Feb 18, 2014

Hey @bendrucker,
In my project, maybe for convinence, we have lots of things done in model.fetch,
such as some calculating...
I come up with an idea, to avoid unnecessary DB calls as you mentioned:
Could I regard the model.fetch as "[sync] + calculate", and write the code like:

// db.Model
fetch: function(options){
  if (options['free']) return Promise.resolve(this);
  return db.Model.__super__.fetch.call(this, options);
}
// db.Collection
fetch: function(options){
  options = options || {};
  var forModel = options['each'];
  forModel['free'] = true;
  return db.Collection.__super__.fetch.call(this, options)
    .then(function(collection) {
      return collection.invokeThen('fetch', forModel)
        .then(function () {
            return collection;
        });
    });
}
@bogus34

This comment has been minimized.

Copy link
Author

commented Feb 18, 2014

I'm convinced that we have to threat fetching a collection just as an another way to fetch a model. After all when I fetch a collection I got a model that was retrieved from database.

I'm just inspecting and planing so I haven't REAL example. But @fritx have.
And I can easily invent one.

Maybe I want to implement a sort of id-based cache. So when model is fetched I put it to cache so I can take it back in future.
Or maybe I want to write something to log when my special model is fetched.

Simplest idea to implemment this is to hook into fetched event. But next in some point I'll use Collection to get a bunch of models and everething goes wrong. Btw, collection may be used implicitly if I load my model in context of relation.

@bendrucker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

A collection is a convenient way to populate and manage a group of models, but not doing operations on each model is pretty consistently implemented. You have invokeThen available for calling model methods no save method on a collection. Right now there's no magic for collections—reads can be done automatically, but things like saves and destroys should use custom queries. The problem here is that collections can potentially have many models and automatically firing lots of event listeners could have fairly serious performance implications. In the case of a cache, you should cache the whole collection or bulk insert all its members. Going and doing that individually is going to take a long time.

@fritx While that will work, I recommend against it. It's not a good idea to have Model.prototype.fetch sometimes fetch the model and other times just run some side-effects. It's brittle and will potentially create headaches for you with future API changes in Bookshelf.

And yes, if you add custom collection behavior you'd need to use that collection explicitly in relations where the model is on the right side of an n-to-many relationship rather than allowing a collection to be implicitly created from the model.

It would be really helpful to see actual code here. Firing fetched events on every model in a collection is easy and I'll look into it at some point. But I'd generally consider it an anti-pattern. Happy to help figure out the right way to do whatever you need.

@bogus34

This comment has been minimized.

Copy link
Author

commented Feb 18, 2014

To summarize.

My main point is that Model is actually fetched from database even if it is loaded by Collection. So it looks logical if it will fire apropriate events.

Your opposit point is that firing many events may lead to a performance problems.

Am I correct?
Anyway, it's worth to be explicitly documented.

Thank you!

@bendrucker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

Roughly correct. There's performance issue, but also the general principle that it's way easier for you to add custom behavior than to strip out something we enforce. If a collection.fetch triggers load cycle events on all models, it's pretty hard to prevent that if you don't need it. There'd have to be an options flag and that's a nightmare to document and generally bad practice.

Whereas if you want to add in this behavior, it's easy. Here's how you and @fritx should be doing it:

collection.fetch()
  .then(function (collection) {
    return collection.models;
  })
  .map(function (model) {
    return model.triggerThen('fetched');
  })
  .return(collection);

See the Bluebird API docs for more on functional programming w/ promises. You should make sure your fetched handlers only contain basic sync operations otherwise you'll slow down your application significantly. I'm starting to map out an FAQ/recipes sections for the revamped docs w/ common problems. I'll include this there.

@bogus34

This comment has been minimized.

Copy link
Author

commented Feb 18, 2014

There's performance issue, but also the general principle that it's way easier for you to add custom behavior than to strip out something we enforce.

Yes, that makes sense. Thank you!

@bendrucker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2014

You got it! I highly recommend taking a 15 min read through with the Bluebird API. It's a really amazing library and functional transformations (maps, filters, reducers, etc.) of promises can be extremely powerful.

@fritx

This comment has been minimized.

Copy link

commented Feb 20, 2014

It's not a good idea to have Model.prototype.fetch sometimes fetch the model and other times just run some side-effects. It's brittle and will potentially create headaches for you with future API changes in Bookshelf.

It makes sense. Thanks!
I find I had taken out a how-bad-it-is sample :[ @bogus34
And not until @bendrucker shared his code, I have found that Promise is so great! :)

@diorahman

This comment has been minimized.

Copy link

commented Feb 7, 2016

Is it possible to override the query builder at fetching? Tried it but it was failed. I tried to extend the https://github.com/lanetix/node-bookshelf-soft-delete to have checking when withRelated is specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.