search

Everyplay / serverbone

Backbone node.js additions to serve backbone collections/models over express
MIT License
5 stars 5 forks source link

Feature/resource cleanup #20

Closed kosmikko closed 9 years ago

kosmikko commented 9 years ago

This PR fixes #18.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.45%) when pulling ba3b976e0ec266e6c7fbf1e8af1934c18ccc82b0 on feature/resource-cleanup into dc9529ac85b0b4d1a21c7dae92f5e6a828a2f59e on master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+1.81%) when pulling 3f75b23d1afeed0f94d2deaa56deab2eb9d1d519 on feature/resource-cleanup into dc9529ac85b0b4d1a21c7dae92f5e6a828a2f59e on master.

mzabaluev commented 9 years ago

Looks good apart from the comment above. Somewhat coincidentally, I've been thinking of how to make the interplay between fetch and use less implicit and fragile. There seems to be a proliferation of 'fetchXXXDependencies' methods, and subsequent operations on the model may fail if you've fetched it the wrong way.

So I think there might be use for a public async method to fetch a relation on demand:

model.fetchRelation('admirers').then(function (admirers) {
   // ...
});

Then, for example, the ACL check would, in most general form, be an async operation done in preFetch/preSave/preDelete (the current .canAccess and .getRoles could be retained for the simple default implementation), and there won't be need for fetchACLDependencies. Same for other code that currently relies on internal properties populated by fetchAll and the like.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+1.81%) when pulling 7ec0ed6e44d46472d9ecbfc5d9529e548d74c749 on feature/resource-cleanup into dc9529ac85b0b4d1a21c7dae92f5e6a828a2f59e on master.