Skip to content

Commit

Permalink
DAO.create: don't return the instance
Browse files Browse the repository at this point in the history
Simplify DataAccessObject.create() and stop returning the
instance/array of instances. Users should always use callback (or
returned promise) to get the instance(s) created.
  • Loading branch information
bajtos committed Apr 29, 2016
1 parent 89bf942 commit a1d852d
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 13 deletions.
12 changes: 12 additions & 0 deletions 3.0-RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,15 @@ checks `if (ctx.instance)` then you can remove this condition together with
the branch that follows.

See also the commit [30283291](https://github.com/strongloop/loopback-datasource-juggler/commit/30283291?w=1)

## DAO.create no longer returns the instance(s) created

While implementing support for promises in `DAO.create`, we found that this
method returns the instance object synchronously. This is inconsistent with
the usual convention of accessing the result via callback/promise and it may
not work correctly in cases where some fields like the `id` property are
generated by the database.

We have changed the API to be consistent with other DAO methods: when invoked
with a callback argument, the method does not return anything. When invoked
without any callback, a promise is returned.
7 changes: 2 additions & 5 deletions lib/dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ DataAccessObject.create = function(data, options, cb) {
}
cb(errors, data);
});
return data;
return;
}

var enforced = {};
Expand Down Expand Up @@ -402,10 +402,7 @@ DataAccessObject.create = function(data, options, cb) {
}, obj, cb);
}

// Does this make any sense? How would chaining be used here? -partap

// for chaining
return cb.promise || obj;
return cb.promise;
};

function stillConnecting(dataSource, obj, args) {
Expand Down
15 changes: 7 additions & 8 deletions test/manipulation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,12 @@ describe('manipulation', function() {

});

it('should return instance of object', function(done) {
it('should not return instance of object', function(done) {
var person = Person.create(function(err, p) {
p.id.should.eql(person.id);
should.exist(p.id);
if (person) person.should.not.be.an.instanceOf(Person);
done();
});
should.exist(person);
person.should.be.an.instanceOf(Person);
should.not.exist(person.id);
});

it('should not allow user-defined value for the id of object - create', function(done) {
Expand Down Expand Up @@ -235,7 +233,8 @@ describe('manipulation', function() {
{ name: 'Boltay' },
{},
];
Person.create(batch, function(e, ps) {
var res = Person.create(batch, function(e, ps) {
if (res) res.should.not.be.instanceOf(Array);
should.not.exist(e);
should.exist(ps);
ps.should.be.instanceOf(Array);
Expand All @@ -254,8 +253,8 @@ describe('manipulation', function() {
persons.should.have.lengthOf(batch.length);
persons[0].errors.should.be.false;
done();
}).should.be.instanceOf(Array);
}).should.have.lengthOf(3);
});
});
});

it('should create batch of objects with beforeCreate', function(done) {
Expand Down

0 comments on commit a1d852d

Please sign in to comment.