opened
junBryl
username_0
Model.PublishDestroy throws an error and exits sails, if Model has a collection
issue
created
junBryl
username_0
Hello @karsasmus,

`destroy()` also returns the object that was destroyed, even if it was not stated in the docs.
I also tried using only the err object but the same thing happens.

```
  Job.destroy({
            id: req.param('id')
        }).exec(function destroy(err) {

            Job.publishDestroy(req.param('id'), undefined, {
                previous: {
                    title: 'test'
                }
            });

            return res.ok();
        });

```
It seems the problem is in `publishDestroy()`. It does not accept `undefined` if the model has a collection.
comment
created
mikermcneil
username_1
@username_0 Thanks for the heads up- looking into it
comment
created
sgress454
username_2
@username_0 `.publishDestroy()` does expect that if the `previous` option is provided, that it is a fully-populated record.  This way it can do any `publishUpdate` or `publishRemove` calls necessary to inform subscribers to associated records that the association no longer exists.  But, it's not clear in the documentation that `previous` is meant to be used this way (and in any case it ought to fail more gracefully).

If someone wants to do a pull request to make `publishDestroy` fail gracefully if it can't find a property, here's the line that needs to be fixed: https://github.com/balderdashy/sails/blob/0.12/lib/hooks/pubsub/index.js#L818
It just needs to check that `previous[association.alias]` exists before accessing `previous[association.alias].length`.

In the meantime, to work around this in the original example, you'd just need to add an empty array to the `previous` value for any associated collections a Job might have, e.g. if it has `users` and `tasks` collections:

```
previous: {
  title: job[0].title,
  users: [],
  tasks: []
}
```
  
Also note that the ability to call these resourceful pubsub methods is being removed in Sails 1.0, precisely because their usage was confusing!
comment
created
junBryl
username_0
@username_1 , @username_2 Thank you guys for the response. Finally, I can make it work. Thanks. haha.

Too bad it will be removed in 1.0. It was working, the lack of documentation and meaningful error logs is what made it confusing to use.
comment
created
mikermcneil
username_1
The inconvenient truth of this new approach is that you have to enforce consistency of the data you send in your broadcasted messages-- but since it is explicit and self-documenting, you can use any of the same approaches you'd use anywhere else in Sails.  For example, you can make your own `Job.publishCancel()` model method:

```javascript
/**
 * Job.publishCancel()
 *
 * Broadcast a message to connected socket clients who are subscribed to
 * any of a particular set of jobs, letting them know the ids of all of the jobs
 * being canceled.
 * 
 * @param {Array} ids
 *     An array of job ids (numbers).
 * @param {Ref?} reqToOmitMaybe
 *     Optional.  A virtual socket request (`req`) that, if specified, will be
 *     omitted from this broadcast.
 *
 * @broadcasts {Dictionary}
 *     @property {String} verb
 *     @property {Array} ids
 */
publishCancel: function (ids, reqToOmitMaybe){
  Job.publish(ids, {
    verb: 'canceled',
    ids: ids
  }, reqToOmitMaybe);
}
```

----------------------------

All that said, if you would prefer to be able to continue to use the CRUD-oriented RPS methods, there are a few different ways to do that:

1. define your own `publishDestroy()`, etc. as default model methods in `config/models.js` _(This is the best solution, I think)_
2. override the `pubsub` hook by copying the code in Sails v0.12 _(bad idea IMO -- you'd have to pin your SVRs if you aren't already, and then any time you upgrade, even a patch release, you'd have to check that everything worked first, since you'd be completely overriding the pubsub hook, and could potentially have stuff that gets out of date)_
3. publish a new hook (e.g. `v012-pubsub`) that _just_ includes the methods that were removed.  _(to me, that seems like a good idea for short-term compatibility, but could still get confusing in the long run)_


Anyway, sorry for the long-winded post (I've been up all night and got ramblesome).  I hope it helps provide a bit of extra background anyway!  And who knows, it might help other folks with similar questions about this change in Sails v1.  

And if you want more info on any of the workarounds I just mentioned, just ask in here -- I'll try my best to come back through issues again later this week.
comment
closed
junBryl
username_0
issue
created
junBryl
username_0
@username_1 No problem, it's fine.
comment
reopened
junBryl
username_0
Model.PublishDestroy throws an error and exits sails, if Model has a collection
issue
created
junBryl
username_0
@username_1 
I'm in favor with the changes.
comment
created
junBryl
username_0
@username_1  I'm in favor with the changes.
comment