mediasuitenz / tech-stack

Media Suite's evolving tech stack
6 stars 0 forks source link

Recommendations about loopback prototype methods for custom end-points #11

Closed JonForest closed 8 years ago

JonForest commented 8 years ago

I think we should favour prototype over static methods for Model endpoints where we are passing in an :id argument.

This PR is for discussion around that topic.

ewen commented 8 years ago

LGTM 👍 I didn't know you could access them like this by setting isStatic to false, definitely saves boilerplate.

I also think it's worth having a convention of assigning this in the first line of the function for prototype methods.

Worksite.prototype.startWork = function (data, cb) {
  const instance = this
  ...
}
JonForest commented 8 years ago

@ewen You think instance is better than the model name? e.g.

const worksite = this

?

ewen commented 8 years ago

@JonForest I'm OK with either, I've been using instance I think cause it's always consistent.

pnw commented 8 years ago

I'd say only use an alias if you use a meaningful name, like worksite or permit or notice or something. Otherwise, instance and this are both equally as general/ambiguous and I'd rather just use this.

markstuart commented 8 years ago

Another thing to note is that the slc command line tools provide a pretty good custom remote method generator. It asks as part of the the interactive process if you want the method to be static or not, and generates the remote definition into the model JSON file. You then need to create the method handler in the model js file.

ewen commented 8 years ago

@pnw I think it's more than just a naming thing, assigning it first removes any thinking about the this context later in the function.

Agree on the naming - may as well make the convention a specific name rather than a generic one.

anotheredward commented 8 years ago

Sweet job, I've added the requested parts to the document, let's get this merged in!

anotheredward commented 8 years ago

Cool, this has been discussed and agreed upon. Merging!