Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 164 forks source link

All Roles Publication In README #302

Closed leprekon91 closed 4 years ago

leprekon91 commented 4 years ago

Shouldn't the publication in the README file under "Changes between 2.x and 3.0" and "Installing", return this.ready() and not call it?

Meteor.publish(null, function () {
  if (this.userId) {
    return Meteor.roleAssignment.find({ 'user._id': this.userId });
  } else {
    this.ready() // --> should be return this.ready();
  }
})
leprekon91 commented 4 years ago

Actually, even better solution would be:

Meteor.publish(null, function() {
  if (this.userId) {
    return Meteor.roleAssignment.find({ 'user._id': this.userId });
  }
  return this.ready();
});
SimonSimCity commented 4 years ago

Thanks for pointing out the missing semicolon.

Please elaborate why the second of your code snippets would be a better solution. To me, this is an opinionated choice of which I like the first (not necessarily for not using the return, but specially for leaving the else in place).

copleykj commented 4 years ago

I personally see nothing wrong here.. The current code is clean and intention is clear. It's not necessary to return this.ready() from a publication, only to call the function.

leprekon91 commented 4 years ago

I had no idea that you can invoke the ready callback and get the same result on the publication. I thought that the function is actually invoked on the client after the ready signal is received from the server, as a way to tell the client it is done sending the current batch of docs.

SimonSimCity commented 4 years ago

Well, the function Meteor.publish() is only available on the server - not on the client (https://docs.meteor.com/api/pubsub.html#Meteor-publish). The client only subscribes to a publication using Meteor.subscribe() (https://docs.meteor.com/api/pubsub.html#Meteor-subscribe).

leprekon91 commented 4 years ago

I understand that. I just thought that instead of an array of cursors the publication will return a callback function when ready (this.ready()), then the client calls this function.

Anyway, I thought that this would be a nicer change because I have a prettier plugin that just won't shut up about this. And I never knew that you can just call this.ready(), and get the same results. Thought maybe I'll share my pain. :-)

SimonSimCity commented 4 years ago

Thanks for the feedback though 👍 I really appreciate that you took the time 😃

copleykj commented 4 years ago

I have a prettier plugin that just won't shut up about this.

@leprekon91 This sounds like the consistent-returns rule for eslint. Personally, I find it to be completely useless and annoying and therefore disable it.

leprekon91 commented 4 years ago

Yeah, but I prefer it that way actually... Feels cleaner and keeps my boss happy during code reviews...