From f2e08f5c55c480d8457cfa579d8d4bb1c31ff03d Mon Sep 17 00:00:00 2001 From: Tim Judkins Date: Wed, 24 Jan 2024 10:48:23 -0800 Subject: [PATCH 1/5] Add failing test --- package-lock.json | 4 ++-- tests/lib/traverse.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4c2e019..e37a1e5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "chrome-types", + "name": "chrome-types-schema-change", "lockfileVersion": 2, "requires": true, "packages": { @@ -32,7 +32,7 @@ "check-code-coverage": "^1.10.0" }, "engines": { - "node": "16" + "node": ">=16" } }, "node_modules/@babel/code-frame": { diff --git a/tests/lib/traverse.js b/tests/lib/traverse.js index 72d7043..a546c09 100644 --- a/tests/lib/traverse.js +++ b/tests/lib/traverse.js @@ -152,6 +152,37 @@ test('expandFunctionParams returns', t => { }); +test('expandFunctionParams returns_async does_not_support_promises', t => { + const filter = () => true; + const tc = new traverse.TraverseContext(filter); + + /** @type {chromeTypes.TypeSpec} */ + const returnsAsyncFunction = { + type: 'function', + parameters: [ + { type: 'string', name: 's' }, + ], + returns_async: { + name: 'callback', + type: 'function', + does_not_support_promises: 'For testing', + parameters: [{ type: 'number', name: 'whatever' }], + }, + }; + + const returnsAsyncFunctionExpansions = tc.expandFunctionParams( + cloneObject(returnsAsyncFunction), + 'api:test', + ); + // Since this does not support promises, we expect to only get the callback signature. + t.deepEqual(returnsAsyncFunctionExpansions, [ + [ + { type: 'void', name: 'return' }, + { type: 'string', name: 's' }, + expectedOptionalReturnsAsync, + ] + ]); +}); test('expandFunctionParams returns_async', t => { const filter = () => true; @@ -198,6 +229,7 @@ test('expandFunctionParams returns_async', t => { }); + test('filter', t => { /** @type {(spec: chromeTypes.TypeSpec, id: string) => boolean} */ const filter = (spec, id) => { From 13f12b5389be80d7119a628e48bf62e8f4b468e6 Mon Sep 17 00:00:00 2001 From: Tim Judkins Date: Wed, 24 Jan 2024 13:38:53 -0800 Subject: [PATCH 2/5] Add changes, fix up test --- tests/lib/traverse.js | 40 +++++++++++++++++++--------------------- tools/lib/traverse.js | 42 +++++++++++++++++++++++++++++------------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/tests/lib/traverse.js b/tests/lib/traverse.js index a546c09..bec6c08 100644 --- a/tests/lib/traverse.js +++ b/tests/lib/traverse.js @@ -152,7 +152,7 @@ test('expandFunctionParams returns', t => { }); -test('expandFunctionParams returns_async does_not_support_promises', t => { +test('expandFunctionParams returns_async', t => { const filter = () => true; const tc = new traverse.TraverseContext(filter); @@ -165,17 +165,29 @@ test('expandFunctionParams returns_async does_not_support_promises', t => { returns_async: { name: 'callback', type: 'function', - does_not_support_promises: 'For testing', parameters: [{ type: 'number', name: 'whatever' }], }, }; + // Because the expansion supports a Promise if this is missing, we expect it to be passed back to + // us with `optional: true`. + const expectedOptionalReturnsAsync = /** @type {chromeTypes.NamedTypeSpec} */ ( + { ...returnsAsyncFunction.returns_async, optional: true } + ); + const returnsAsyncFunctionExpansions = tc.expandFunctionParams( cloneObject(returnsAsyncFunction), 'api:test', ); - // Since this does not support promises, we expect to only get the callback signature. t.deepEqual(returnsAsyncFunctionExpansions, [ + [ + { + $ref: 'Promise', + name: 'return', + value: ['return', { name: 'whatever', type: 'number' }], + }, + { type: 'string', name: 's' }, + ], [ { type: 'void', name: 'return' }, { type: 'string', name: 's' }, @@ -184,7 +196,7 @@ test('expandFunctionParams returns_async does_not_support_promises', t => { ]); }); -test('expandFunctionParams returns_async', t => { +test('expandFunctionParams returns_async does_not_support_promises', t => { const filter = () => true; const tc = new traverse.TraverseContext(filter); @@ -197,39 +209,25 @@ test('expandFunctionParams returns_async', t => { returns_async: { name: 'callback', type: 'function', + does_not_support_promises: 'For testing', parameters: [{ type: 'number', name: 'whatever' }], }, }; - // Because the expansion supports a Promise if this is missing, we expect it to be passed back to - // us with `optional: true`. - const expectedOptionalReturnsAsync = /** @type {chromeTypes.NamedTypeSpec} */ ( - { ...returnsAsyncFunction.returns_async, optional: true } - ); - const returnsAsyncFunctionExpansions = tc.expandFunctionParams( cloneObject(returnsAsyncFunction), 'api:test', ); + // Since this does not support promises, we expect to only get the callback signature. t.deepEqual(returnsAsyncFunctionExpansions, [ - [ - { - $ref: 'Promise', - name: 'return', - value: ['return', { name: 'whatever', type: 'number' }], - }, - { type: 'string', name: 's' }, - ], [ { type: 'void', name: 'return' }, { type: 'string', name: 's' }, - expectedOptionalReturnsAsync, + { ...returnsAsyncFunction.returns_async }, ] ]); }); - - test('filter', t => { /** @type {(spec: chromeTypes.TypeSpec, id: string) => boolean} */ const filter = (spec, id) => { diff --git a/tools/lib/traverse.js b/tools/lib/traverse.js index 123b6ab..b0a1a17 100644 --- a/tools/lib/traverse.js +++ b/tools/lib/traverse.js @@ -146,6 +146,24 @@ export class TraverseContext { if (!returns_async) { return undefined; } + + /** @type {chromeTypes.TypeSpec} */ + let callbackParameter = { + ...returns_async, + type: 'function', + }; + // If this signature doesn't support promises we just convert the "returns_async" field to a + // callback. + if (returns_async.does_not_support_promises) { + return { + withCallback: { + ...clone, + parameters: [...spec.parameters ?? [], callbackParameter], + }, + }; + } + + // Otherwise this is a promise signature, so we can do a few checks to make sure it is valid. if (spec.returns) { throw new Error(`got returns_async and returns on function spec: ${JSON.stringify(spec)}`); } @@ -168,15 +186,8 @@ export class TraverseContext { }; } - // We convert the "returns_async" field to a callback (it has all the same values). - // Force it to be optional: by definition if it's omitted then we'll have a Promise-returning - // version. This isn't done correctly in the source. - /** @type {chromeTypes.TypeSpec} */ - const callbackParameter = { - ...returns_async, - type: 'function', - optional: true, - }; + // For promise supporting functions, the callback in inheriently optional, so we force that here. + callbackParameter.optional = true; return { withPromise: { @@ -198,7 +209,7 @@ export class TraverseContext { /** * Chrome supports "early" optional parameters, as well as a special "returns_async" parameter - * which actually means that this returns a `Promise` OR supports a callback. + * which actually means that this API may return a `Promise` OR supports a callback. * * This expands all possible combinations for rendering as multiple signatures. The return type * includes the signature's return in the 0th position, followed by all parameters. @@ -208,12 +219,17 @@ export class TraverseContext { * @return {[chromeTypes.NamedTypeSpec, ...chromeTypes.NamedTypeSpec[]][]} */ expandFunctionParams(spec, id) { - // If this is actually a Promise-supporting API, then we call ourselves again to support both - // callback and Promise-based versions. + // If this function uses "returns_asyc", expand it out and call ourselves again to generate + // the valid signatures (either just callback or Promise and callback). const expanded = this._maybeExpandFunctionReturnsAsync(spec); if (expanded) { + if (expanded.withPromise && expanded.withCallback) { + return [ + ...this.expandFunctionParams(expanded.withPromise, id), + ...this.expandFunctionParams(expanded.withCallback, id), + ]; + } return [ - ...this.expandFunctionParams(expanded.withPromise, id), ...this.expandFunctionParams(expanded.withCallback, id), ]; } From 5e0a28faa864520898124a61a47e2cd0fc2740a7 Mon Sep 17 00:00:00 2001 From: Tim Judkins Date: Tue, 30 Jan 2024 13:55:58 -0800 Subject: [PATCH 3/5] Restore older file version --- package-lock.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index e37a1e5..4c2e019 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,5 +1,5 @@ { - "name": "chrome-types-schema-change", + "name": "chrome-types", "lockfileVersion": 2, "requires": true, "packages": { @@ -32,7 +32,7 @@ "check-code-coverage": "^1.10.0" }, "engines": { - "node": ">=16" + "node": "16" } }, "node_modules/@babel/code-frame": { From 4038e529538975f3b85c321c8bdb7978aef5f68f Mon Sep 17 00:00:00 2001 From: Tim Judkins Date: Tue, 30 Jan 2024 13:56:28 -0800 Subject: [PATCH 4/5] Add in does_not_support_promises field --- types/chrome.d.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/types/chrome.d.ts b/types/chrome.d.ts index 317f973..5bfafe4 100644 --- a/types/chrome.d.ts +++ b/types/chrome.d.ts @@ -156,6 +156,9 @@ export interface TypeSpec extends SharedSpec { // only for top-level namespace types noinline_doc?: boolean | 'True'; + // only for returns_async types + does_not_support_promises?: string; + // special to mark converted-from-event-to-property, not part of definition _event?: boolean; } From a1ed8de5b2ad7e1e48cc913ed82bca3a0e581816 Mon Sep 17 00:00:00 2001 From: Tim Judkins Date: Tue, 30 Jan 2024 13:57:16 -0800 Subject: [PATCH 5/5] Simplify and expand comment --- tools/lib/traverse.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tools/lib/traverse.js b/tools/lib/traverse.js index b0a1a17..50d26b4 100644 --- a/tools/lib/traverse.js +++ b/tools/lib/traverse.js @@ -219,17 +219,18 @@ export class TraverseContext { * @return {[chromeTypes.NamedTypeSpec, ...chromeTypes.NamedTypeSpec[]][]} */ expandFunctionParams(spec, id) { - // If this function uses "returns_asyc", expand it out and call ourselves again to generate - // the valid signatures (either just callback or Promise and callback). + if (!spec) { + return []; + } + + // If this function uses "returns_async", expand it out and call ourselves again to generate + // the valid signatures for both Promise and callback versions. Note: a few functions with + // asynchronous returns don't support a promise version, in which case withPromise is + // undefined here and will return an empty array (handled just above this). const expanded = this._maybeExpandFunctionReturnsAsync(spec); if (expanded) { - if (expanded.withPromise && expanded.withCallback) { - return [ - ...this.expandFunctionParams(expanded.withPromise, id), - ...this.expandFunctionParams(expanded.withCallback, id), - ]; - } return [ + ...this.expandFunctionParams(expanded.withPromise, id), ...this.expandFunctionParams(expanded.withCallback, id), ]; }