From 3e792b2cc234bdb842e4354a5ec53658c418c580 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:01:30 -0700 Subject: [PATCH 1/8] Use `extract()`. --- src/util/export/BaseValueVisitor.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/util/export/BaseValueVisitor.js b/src/util/export/BaseValueVisitor.js index f88a0371a..b8e9fecdd 100644 --- a/src/util/export/BaseValueVisitor.js +++ b/src/util/export/BaseValueVisitor.js @@ -71,14 +71,10 @@ export class BaseValueVisitor { await visitEntry.promise; } - if (visitEntry.ok) { - // Note: If `result` is a promise, this will cause the caller to - // ultimately receive the resolved/rejected value of it. See the header - // comment for details. - return visitEntry.result; - } else { - throw visitEntry.error; - } + // Note: If `visitEntry.result` is a promise, this will cause the caller to + // ultimately receive the resolved/rejected value of it. See the header + // comment for details. + return visitEntry.extract(); } /** @@ -583,13 +579,19 @@ export class BaseValueVisitor { * an error indicating that the visit is still in progress. */ extract() { + /* c8 ignore start */ if (this.ok === null) { + // The code that calls this should only end up doing so when the + // instance's visit is known to be finished. if (this.promise === null) { - throw new Error('Visit not yet started.'); + throw new Error('Shouldn\'t happen: Visit not yet started.'); } else { - throw new Error('Visit not yet complete.'); + throw new Error('Shouldn\'t happen: Visit not yet complete.'); } - } else if (this.ok) { + } + /* c8 ignore end */ + + if (this.ok) { return this.result; } else { throw this.error; From 82f9572652692f4f36b4e329adcf6d66b9950f4d Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:09:55 -0700 Subject: [PATCH 2/8] Simplify. --- src/util/export/BaseValueVisitor.js | 49 ++++++++++++++--------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src/util/export/BaseValueVisitor.js b/src/util/export/BaseValueVisitor.js index b8e9fecdd..2336376da 100644 --- a/src/util/export/BaseValueVisitor.js +++ b/src/util/export/BaseValueVisitor.js @@ -86,25 +86,7 @@ export class BaseValueVisitor { * processed the original `value`. */ visitSync() { - const visitEntry = this.#visitNode(this.#value); - - switch (visitEntry.ok) { - case null: { - throw new Error('Could not complete synchronously.'); - } - case false: { - throw visitEntry.error; - } - case true: { - return visitEntry.result; - } - /* c8 ignore start */ - default: { - // This is indicative of a bug in this class. - throw new Error('Shouldn\'t happen.'); - } - /* c8 ignore stop */ - } + return this.#visitNode(this.#value).extract(true); } /** @@ -573,23 +555,38 @@ export class BaseValueVisitor { /** * Extracts the result or error of a visit. * + * @param {boolean} [possiblyUnfinished] Should it be an _expected_ + * possibility that the visit has been started but not finished? * @returns {*} The successful result of the visit, if it was indeed * successful. * @throws {Error} The error resulting from the visit, if it failed; or * an error indicating that the visit is still in progress. */ - extract() { - /* c8 ignore start */ + extract(possiblyUnfinished = false) { if (this.ok === null) { - // The code that calls this should only end up doing so when the - // instance's visit is known to be finished. + // This is indicative of a bug in this class: Either the call should + // know the visit is finished, or it should be part of an API that + // exposes the possibility of an unfinished visit (in which case, it + // should have passed `true`). + /* c8 ignore start */ if (this.promise === null) { + // This is indicative of a bug in this class: This method should never + // get called before a visit is started. throw new Error('Shouldn\'t happen: Visit not yet started.'); - } else { - throw new Error('Shouldn\'t happen: Visit not yet complete.'); } + /* c8 ignore end */ + + if (possiblyUnfinished) { + throw new Error('Visit did not complete synchronously.'); + } + + // This is indicative of a bug in this class: If the caller thinks its + // possible that the visit hasn't finished, it should have passed `true` + // to this method. + /* c8 ignore start */ + throw new Error('Shouldn\'t happen: Visit not yet complete.'); + /* c8 ignore end */ } - /* c8 ignore end */ if (this.ok) { return this.result; From 4dda60afdc62f5848af8fd36adb521c91bfa790f Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:11:59 -0700 Subject: [PATCH 3/8] Add a test. --- src/util/tests/BaseValueVisitor.test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/util/tests/BaseValueVisitor.test.js b/src/util/tests/BaseValueVisitor.test.js index 8f5cbbe67..ba1bee2cd 100644 --- a/src/util/tests/BaseValueVisitor.test.js +++ b/src/util/tests/BaseValueVisitor.test.js @@ -150,10 +150,14 @@ describe('visitSync()', () => { }); test('throws the error which was thrown synchronously by an `_impl_visit*()` method', () => { - const vv = new SubVisit(123n); - + const vv = new SubVisit(123n); expect(() => vv.visitSync()).toThrow('Nope!'); }); + + test('throws the right error if the visit did not complete synchronously', () => { + const vv = new SubVisit(true); + expect(() => vv.visitSync()).toThrow('Visit did not complete synchronously.'); + }); }); describe('visitWrap()', () => { From 63d55a38e8025260e8df8f4c9eb514589b298673 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:22:47 -0700 Subject: [PATCH 4/8] Simplify. --- src/util/export/BaseValueVisitor.js | 60 ++++++++++++++++------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/util/export/BaseValueVisitor.js b/src/util/export/BaseValueVisitor.js index 2336376da..519b952ac 100644 --- a/src/util/export/BaseValueVisitor.js +++ b/src/util/export/BaseValueVisitor.js @@ -63,18 +63,7 @@ export class BaseValueVisitor { * processed the original `value`. */ async visit() { - const visitEntry = this.#visitNode(this.#value); - - if (visitEntry.ok === null) { - // Wait for the visit to complete, either successfully or not. This should - // never throw. - await visitEntry.promise; - } - - // Note: If `visitEntry.result` is a promise, this will cause the caller to - // ultimately receive the resolved/rejected value of it. See the header - // comment for details. - return visitEntry.extract(); + return this.#visitNode(this.#value).extractAsync(false); } /** @@ -100,19 +89,7 @@ export class BaseValueVisitor { * processed the original `value`. */ async visitWrap() { - const visitEntry = this.#visitNode(this.#value); - - if (visitEntry.ok === null) { - // Wait for the visit to complete, either successfully or not. This should - // never throw. - await visitEntry.promise; - } - - if (visitEntry.ok) { - return new BaseValueVisitor.WrappedResult(visitEntry.result); - } else { - throw visitEntry.error; - } + return this.#visitNode(this.#value).extractAsync(true); } /** @@ -553,7 +530,38 @@ export class BaseValueVisitor { } /** - * Extracts the result or error of a visit. + * Extracts the result or error of a visit, always first waiting until after + * the visit is complete. + * + * Note: If `visitEntry.result` is a promise and `wrapResult` is passed as + * `false`, this will cause the caller to ultimately receive the fulfilled + * (resolved/rejected) value of that promise and not the result promise per + * se. This is the crux of the difference between {link #visit} and + * {@link #visitWrap} (see which). + * + * @param {boolean} wrapResult Should a successful result be wrapped? + * @returns {*} The successful result of the visit, if it was indeed + * successful. + * @throws {Error} The error resulting from the visit, if it failed. + */ + async extractAsync(wrapResult) { + if (this.ok === null) { + // Wait for the visit to complete, either successfully or not. This + // should never throw. + await this.promise; + } + + if (this.ok) { + return wrapResult + ? new BaseValueVisitor.WrappedResult(this.result) + : this.result; + } else { + throw this.error; + } + } + + /** + * Synchronously extracts the result or error of a visit. * * @param {boolean} [possiblyUnfinished] Should it be an _expected_ * possibility that the visit has been started but not finished? From 3333fa8c5014495b6155eae81ee2c4fc0c20c5be Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:23:17 -0700 Subject: [PATCH 5/8] Naming harmony. --- src/util/export/BaseValueVisitor.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/export/BaseValueVisitor.js b/src/util/export/BaseValueVisitor.js index 519b952ac..243e26c6b 100644 --- a/src/util/export/BaseValueVisitor.js +++ b/src/util/export/BaseValueVisitor.js @@ -75,7 +75,7 @@ export class BaseValueVisitor { * processed the original `value`. */ visitSync() { - return this.#visitNode(this.#value).extract(true); + return this.#visitNode(this.#value).extractSync(true); } /** @@ -466,7 +466,7 @@ export class BaseValueVisitor { // There was at least one promise returned from visiting an element. return (async () => { for (const name of promNames) { - result[name] = (await result[name]).extract(); + result[name] = (await result[name]).extractSync(); } return result; @@ -570,7 +570,7 @@ export class BaseValueVisitor { * @throws {Error} The error resulting from the visit, if it failed; or * an error indicating that the visit is still in progress. */ - extract(possiblyUnfinished = false) { + extractSync(possiblyUnfinished = false) { if (this.ok === null) { // This is indicative of a bug in this class: Either the call should // know the visit is finished, or it should be part of an API that From 5bd6f576777be9bc49273d5e8c8ffe3d7cc105f4 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:26:31 -0700 Subject: [PATCH 6/8] Add tests. --- src/util/tests/BaseValueVisitor.test.js | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/util/tests/BaseValueVisitor.test.js b/src/util/tests/BaseValueVisitor.test.js index ba1bee2cd..8fac74856 100644 --- a/src/util/tests/BaseValueVisitor.test.js +++ b/src/util/tests/BaseValueVisitor.test.js @@ -272,7 +272,7 @@ describe('_prot_visitArrayProperties()', () => { expect(got).toEqual(expected); }); - test('handles symbol properties', () => { + test('handles synchronously-visitable symbol properties', () => { const SYM1 = Symbol.for('x'); const SYM2 = Symbol('y'); const orig = [123]; @@ -290,6 +290,25 @@ describe('_prot_visitArrayProperties()', () => { expect(got[SYM1]).toBe('234'); expect(got[SYM2]).toBe('321'); }); + + test('handles asynchronously-visitable symbol properties', async () => { + const SYM1 = Symbol.for('x'); + const SYM2 = Symbol('y'); + const orig = [123]; + orig[SYM1] = true; + orig[SYM2] = false; + + const expected = ['123']; + expected[SYM1] = 'true'; + expected[SYM2] = 'false'; + + const vv = new SubVisit(orig); + const got = await vv.visit(); + expect(got).toBeArrayOfSize(1); + expect(got[0]).toBe('123'); + expect(got[SYM1]).toBe('true'); + expect(got[SYM2]).toBe('false'); + }); }); describe('_prot_visitObjectProperties()', () => { From 080e3c3583873df85184a616d8bb5feb97a0c937 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:29:35 -0700 Subject: [PATCH 7/8] Add tests. --- src/util/tests/BaseValueVisitor.test.js | 29 ++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/util/tests/BaseValueVisitor.test.js b/src/util/tests/BaseValueVisitor.test.js index 8fac74856..a56b96012 100644 --- a/src/util/tests/BaseValueVisitor.test.js +++ b/src/util/tests/BaseValueVisitor.test.js @@ -241,6 +241,12 @@ describe('_prot_visitArrayProperties()', () => { expect(() => vv.visitSync()).toThrow('Nope!'); }); + test('asynchronously propagates an error thrown by one of the sub-calls', () => { + const vv = new SubVisit([Symbol('zonk')]); + + expect(vv.visit()).rejects.toThrow('NO'); + }); + test('preserves sparseness', () => { const UND = undefined; const orig = Array(7); @@ -350,7 +356,13 @@ describe('_prot_visitObjectProperties()', () => { expect(() => vv.visitSync()).toThrow('Nope!'); }); - test('handles symbol properties', () => { + test('asynchronously propagates an error thrown by one of the sub-calls', () => { + const vv = new SubVisit({ blorp: Symbol('zonk') }); + + expect(vv.visit()).rejects.toThrow('NO'); + }); + + test('handles synchronously-visitable symbol properties', () => { const SYM1 = Symbol.for('x'); const SYM2 = Symbol('y'); const orig = { @@ -364,4 +376,19 @@ describe('_prot_visitObjectProperties()', () => { expect(got[SYM1]).toBe('234'); expect(got[SYM2]).toBe('321'); }); + + test('handles asynchronously-visitable symbol properties', async () => { + const SYM1 = Symbol.for('x'); + const SYM2 = Symbol('y'); + const orig = { + [SYM1]: true, + [SYM2]: false + }; + + const vv = new SubVisit(orig); + const got = await vv.visit(); + expect(got).toBeObject(); + expect(got[SYM1]).toBe('true'); + expect(got[SYM2]).toBe('false'); + }); }); From b52424b59db58b69ec064152638badf38b0c83a1 Mon Sep 17 00:00:00 2001 From: Dan Bornstein Date: Mon, 30 Sep 2024 17:32:34 -0700 Subject: [PATCH 8/8] Simplify / DRY. --- src/util/export/BaseValueVisitor.js | 37 ++++++++++++----------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/src/util/export/BaseValueVisitor.js b/src/util/export/BaseValueVisitor.js index 243e26c6b..1a0449bd2 100644 --- a/src/util/export/BaseValueVisitor.js +++ b/src/util/export/BaseValueVisitor.js @@ -434,31 +434,24 @@ export class BaseValueVisitor { const isArray = Array.isArray(result); const promNames = []; - for (const name of Object.getOwnPropertyNames(node)) { - if (!(isArray && (name === 'length'))) { - const got = this.#visitNode(node[name]); - if (got.ok === null) { - promNames.push(name); - result[name] = got.promise; - } else if (got.ok) { - result[name] = got.result; - } else { - throw got.error; + const addResults = (iter) => { + for (const name of iter) { + if (!(isArray && (name === 'length'))) { + const got = this.#visitNode(node[name]); + if (got.ok === null) { + promNames.push(name); + result[name] = got.promise; + } else if (got.ok) { + result[name] = got.result; + } else { + throw got.error; + } } } - } + }; - for (const name of Object.getOwnPropertySymbols(node)) { - const got = this.#visitNode(node[name]); - if (got.ok === null) { - promNames.push(name); - result[name] = got.promise; - } else if (got.ok) { - result[name] = got.result; - } else { - throw got.error; - } - } + addResults(Object.getOwnPropertyNames(node)); + addResults(Object.getOwnPropertySymbols(node)); if (promNames.length === 0) { return result;