Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve addPromised with new doc id #305

Merged
merged 5 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions src/Model/mutators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ var RemoveEvent = mutationEvents.RemoveEvent;
var MoveEvent = mutationEvents.MoveEvent;
var promisify = util.promisify;

type ValueCallback<T> = ((error: Error | null | undefined, value: T) => void);

declare module './Model' {
interface Model<T> {
_mutate(segments, fn, cb): void;
Expand Down Expand Up @@ -43,11 +45,11 @@ declare module './Model' {
createNullPromised(subpath: Path, value: any): Promise<void>;
_createNull(segments: Segments, value: any, cb?: ErrorCallback): void;

add(value: any, cb?: ErrorCallback): string;
add(subpath: Path, value: any, cb?: ErrorCallback): string;
add(value: any, cb?: ValueCallback<string>): string;
add(subpath: Path, value: any, cb?: ValueCallback<string>): string;
addPromised(value: any): Promise<string>;
addPromised(subpath: Path, value: any): Promise<string>;
_add(segments: Segments, value: any, cb?: ErrorCallback): string;
_add(segments: Segments, value: any, cb?: ValueCallback<string>): string;

/**
* Deletes the value at this model's path or a relative subpath.
Expand Down Expand Up @@ -404,20 +406,23 @@ Model.prototype.add = function() {
var segments = this._splitPath(subpath);
return this._add(segments, value, cb);
};
Model.prototype.addPromised = promisify(Model.prototype.add);
Model.prototype.addPromised = promisify<string>(Model.prototype.add);

Model.prototype._add = function(segments, value, cb) {
if (typeof value !== 'object') {
var message = 'add requires an object value. Invalid value: ' + value;
cb = this.wrapCallback(cb);
return cb(new Error(message));
let message = 'add requires an object value. Invalid value: ' + value;
const errorCallback = this.wrapCallback(cb);
errorCallback(new Error(message));
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type 'undefined' is not assignable to type 'string'. consider adding to your interface : string | undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericyhwang what do you think of throwing an error here? Optional callback isn't great for the error handling case as this is really argument validation (so before any "work" that would result in callback call) but I'm unclear as to impacts or likelihood of this case. No id to return, and don't like the optional string return for downstream implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW Typescipt is okay with this (incompletely typed), and as this is what already exists in current code, It's what it will stay with for now.

Does highlight another point though... our type sigs allow value: any and not constrained to object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing an error seems reasonable to me. Should be rare to encounter, since most calls to add should be with an argument that's statically known to be an object.

Only concern would be that the caller may not be set up to handle a thrown error.

Either way, let's leave it alone for now. Even if we did want to change it to a thrown error, we should do it in a separate change, and also apply the change to the other mutator methods too.

}
var id = value.id || this.id();

const id = value.id || this.id();
value.id = id;
segments = this._dereference(segments.concat(id));
var model = this;
const model = this;

function add(doc, docSegments, fnCb) {
var previous;
let previous;
craigbeck marked this conversation as resolved.
Show resolved Hide resolved
if (docSegments.length) {
previous = doc.set(docSegments, value, fnCb);
} else {
Expand All @@ -426,10 +431,15 @@ Model.prototype._add = function(segments, value, cb) {
// it being stored in the database by ShareJS
value = doc.get();
}
var event = new ChangeEvent(value, previous, model._pass);
const event = new ChangeEvent(value, previous, model._pass);
model._emitMutation(segments, event);
}
this._mutate(segments, add, cb);

const callbackWithId = (cb != null)
? (err: Error) => { cb(err, id); }
: null;

this._mutate(segments, add, callbackWithId);
return id;
};

Expand Down
23 changes: 23 additions & 0 deletions test/Model/mutators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const {expect} = require('chai');
const {RootModel} = require('../../lib/Model');

describe('mutators', () => {
describe('add', () => {
const guidRegExp = new RegExp(/[a-f0-9]{8}(-[a-f0-9]{4}){3}-[a-f0-9]{12}/);
it('returns created id in callback', () => {
const model = new RootModel();
model.add('_test_doc', {name: 'foo'}, (error, id) => {
expect(error).to.not.exist;
expect(id).not.to.be.undefined;
expect(id).to.match(guidRegExp, 'Expected a GUID-like Id');
});
});

it('resolves promised add with id', async () => {
const model = new RootModel();
const id = await model.addPromised('_test_doc', {name: 'bar'});
expect(id).not.to.be.undefined;
expect(id).to.match(guidRegExp, 'Expected a GUID-like Id');
});
});
});
Loading