Commit 5300b58c authored by Robert Knight's avatar Robert Knight

Refactor Guest#createAnnotation for readability

Convert the `createAnnotation` method of `Guest` to use async/await
instead of manual chaining of Promise operations. This makes it easier
to follow the logic and see that document metadata is assigned to the
annotation before it is broadcast to other parts of the annotator in the
`beforeAnnotationCreated` event. It also makes it easier to external
code (eg. the tests) to determine when `createAnnotation` is "done" and
has finished updating the annotation.

 - Convert `Guest#createAnnotation` to use async/await

 - Update tests for `createAnnotation` to await the returned Promise
   instead of a fixed timeout

 - Add a missing check in `createAnnotation` tests for the argument
   passed to the `beforeAnnotationCreated` event
parent 9cb70c87
...@@ -535,46 +535,36 @@ export default class Guest extends Delegator { ...@@ -535,46 +535,36 @@ export default class Guest extends Delegator {
* @param {Partial<AnnotationData>} annotation - Initial properties of the * @param {Partial<AnnotationData>} annotation - Initial properties of the
* new annotation, other than `target`, `document` and `uri` which are * new annotation, other than `target`, `document` and `uri` which are
* set by this method. * set by this method.
* @return {Promise<AnnotationData>} - The updated annotation with selectors
* and metadata populated.
*/ */
createAnnotation(annotation = {}) { async createAnnotation(annotation = {}) {
const root = this.element;
const ranges = this.selectedRanges ?? []; const ranges = this.selectedRanges ?? [];
this.selectedRanges = null; this.selectedRanges = null;
const getSelectors = range => { const info = await this.getDocumentInfo();
// Returns an array of selectors for the passed range. annotation.document = info.metadata;
return this.anchoring.describe(root, range); annotation.uri = info.uri;
};
const setDocumentInfo = info => {
annotation.document = info.metadata;
annotation.uri = info.uri;
};
const setTargets = ([info, selectors]) => {
// `selectors` is an array of arrays: each item is an array of selectors
// identifying a distinct target.
const source = info.uri;
annotation.target = selectors.map(selector => ({
source,
selector,
}));
};
const info = this.getDocumentInfo();
info.then(setDocumentInfo);
const selectors = Promise.all(ranges.map(getSelectors)); // `selectors` is an array of arrays: each item is an array of selectors
const targets = Promise.all([info, selectors]).then(setTargets); // identifying a distinct target.
const root = this.element;
const selectors = await Promise.all(
ranges.map(range => this.anchoring.describe(root, range))
);
annotation.target = selectors.map(selector => ({
source: info.uri,
selector,
}));
targets.then(() => this.publish('beforeAnnotationCreated', [annotation])); this.publish('beforeAnnotationCreated', [annotation]);
targets.then(() => this.anchor(/** @type {AnnotationData} */ (annotation))); this.anchor(/** @type {AnnotationData} */ (annotation));
if (!annotation.$highlight) { if (!annotation.$highlight) {
this.crossframe?.call('openSidebar'); this.crossframe?.call('openSidebar');
} }
return annotation;
return /** @type {AnnotationData} */ (annotation);
} }
/** /**
......
...@@ -29,10 +29,6 @@ class FakeTextRange { ...@@ -29,10 +29,6 @@ class FakeTextRange {
} }
} }
// A little helper which returns a promise that resolves after a timeout
const timeoutPromise = (millis = 0) =>
new Promise(resolve => setTimeout(resolve, millis));
describe('Guest', () => { describe('Guest', () => {
const sandbox = sinon.createSandbox(); const sandbox = sinon.createSandbox();
let highlighter; let highlighter;
...@@ -609,29 +605,34 @@ describe('Guest', () => { ...@@ -609,29 +605,34 @@ describe('Guest', () => {
}); });
describe('#createAnnotation', () => { describe('#createAnnotation', () => {
it('adds metadata to the annotation object', () => { it('adds metadata to the annotation object', async () => {
const guest = createGuest(); const guest = createGuest();
const annotation = {}; const annotation = {};
guest.createAnnotation(annotation); await guest.createAnnotation(annotation);
return timeoutPromise().then(() => { assert.equal(annotation.uri, fakeDocumentMeta.uri());
assert.equal(annotation.uri, fakeDocumentMeta.uri()); assert.deepEqual(annotation.document, fakeDocumentMeta.metadata);
assert.deepEqual(annotation.document, fakeDocumentMeta.metadata);
});
}); });
it('merges properties of input object into returned annotation', () => { it('merges properties of input object into returned annotation', async () => {
const guest = createGuest(); const guest = createGuest();
let annotation = { foo: 'bar' }; let annotation = { foo: 'bar' };
annotation = guest.createAnnotation(annotation);
annotation = await guest.createAnnotation(annotation);
assert.equal(annotation.foo, 'bar'); assert.equal(annotation.foo, 'bar');
}); });
it('triggers a "beforeAnnotationCreated" event', done => { it('triggers a "beforeAnnotationCreated" event', async () => {
const guest = createGuest(); const guest = createGuest();
guest.subscribe('beforeAnnotationCreated', () => done()); const callback = sinon.stub();
guest.createAnnotation(); guest.subscribe('beforeAnnotationCreated', callback);
const annotation = {};
await guest.createAnnotation(annotation);
assert.calledWith(callback, annotation);
}); });
}); });
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment