Skip to content

Commit

Permalink
Fix promise availability for storage API
Browse files Browse the repository at this point in the history
We began to add promise support for the storage API in Chrome 88 [1],
but this was reverted and we didn't finish implementation until Chrome
95 [2]. This caused some issues with our version detection.

I'm not a huge fan of the approach here, but this felt like a good
balance between a hardcoded exception in the actual generation code and
not overengineering something for this one case.

[1] https://chromiumdash.appspot.com/commit/84869dc2d4381fe85fd71d5f41289de26caeb406
[2] https://chromiumdash.appspot.com/commit/4300dde73a5036bee8d7c00944fda7e8dc5df8d4
  • Loading branch information
oliverdunk committed Feb 11, 2025
1 parent 351aa4d commit 0760d45
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 9 deletions.
2 changes: 1 addition & 1 deletion tools/lib/render-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ export class RenderContext {

// Don't invoke symbol callbacks while we render this, because there's many possible expansions.
this.#skipCallbacks(() => {
const expansions = this.#t.expandFunctionParams(spec, id);
const expansions = this.#t.expandFunctionParams(spec, id, this.#override.isPromiseSupportVisible(spec, id));

// Record all possible expansions, except void, which isn't rendered / does not exist.
expansions.flat().forEach((param) => {
Expand Down
19 changes: 11 additions & 8 deletions tools/lib/traverse.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,13 @@ export class TraverseContext {

/**
* @param {chromeTypes.TypeSpec} spec of the function which itself has returns_async
* @param {boolean} promiseSupportVisible If we should hide the promise return type
* @return {{
* withPromise: chromeTypes.TypeSpec,
* withCallback: chromeTypes.TypeSpec,
* }=}
*/
_maybeExpandFunctionReturnsAsync(spec) {
_maybeExpandFunctionReturnsAsync(spec, promiseSupportVisible) {
const { returns_async, ...clone } = spec;
if (!returns_async) {
return undefined;
Expand All @@ -152,9 +153,10 @@ export class TraverseContext {
...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) {

// If this signature doesn't support promises, or promise support is hidden
// in this version, we just convert the "returns_async" field to a callback.
if (returns_async.does_not_support_promises || !promiseSupportVisible) {
return {
withCallback: {
...clone,
Expand Down Expand Up @@ -216,9 +218,10 @@ export class TraverseContext {
*
* @param {chromeTypes.TypeSpec} spec
* @param {string} id
* @param {boolean} promiseSupportVisible If we should hide the promise return type
* @return {[chromeTypes.NamedTypeSpec, ...chromeTypes.NamedTypeSpec[]][]}
*/
expandFunctionParams(spec, id) {
expandFunctionParams(spec, id, promiseSupportVisible) {
if (!spec) {
return [];
}
Expand All @@ -227,11 +230,11 @@ export class TraverseContext {
// 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);
const expanded = this._maybeExpandFunctionReturnsAsync(spec, promiseSupportVisible);
if (expanded) {
return [
...this.expandFunctionParams(expanded.withPromise, id),
...this.expandFunctionParams(expanded.withCallback, id),
...this.expandFunctionParams(expanded.withPromise, id, promiseSupportVisible),
...this.expandFunctionParams(expanded.withCallback, id, promiseSupportVisible),
];
}

Expand Down
23 changes: 23 additions & 0 deletions tools/override.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ export class EmptyRenderOverride {
return true;
}

isPromiseSupportVisible(spec, id) {
return true;
}

/**
* @return {string | undefined}
*/
Expand Down Expand Up @@ -145,6 +149,25 @@ export class RenderOverride extends EmptyRenderOverride {
return !spec.nodoc;
}

/**
* @param {chromeTypes.TypeSpec} spec
* @param {string} id
*/
isPromiseSupportVisible(spec, id) {
switch (id) {
case 'api:storage.StorageArea.get':
case 'api:storage.StorageArea.set':
case 'api:storage.StorageArea.remove':
case 'api:storage.StorageArea.clear':
case 'api:storage.StorageArea.getBytesInUse':
// We added and then reverted a commit to land promise support in 88.
// The commit which eventually shipped to land this was in Chrome 95.
return !this.#majorVersion || this.#majorVersion >= 95;
default:
return true;
}
}

/**
* @param {string} id
*/
Expand Down
5 changes: 5 additions & 0 deletions types/override.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ export interface RenderOverride {
*/
isVisible(spec: chromeTypes.TypeSpec, id: string): boolean;

/**
* Should we show the fact that this method supports promises?
*/
isPromiseSupportVisible(spec: chromeTypes.TypeSpec, id: string): boolean;

/**
* These are the template overrides for interface definitions within Chrome's extensions codebase.
*
Expand Down

0 comments on commit 0760d45

Please sign in to comment.