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

Definitions for W3C's Web Locks API - browser externs #4156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ecrider
Copy link
Contributor

@ecrider ecrider commented Feb 29, 2024

Added browser externs for Web Locks API - based on definitions from latest W3C draft
. The coverage is already great and API synergy with web workers cannot be overstated. Feel free to poke holes in it.

@lauraharker lauraharker self-assigned this Feb 29, 2024
Copy link
Contributor

@lauraharker lauraharker left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

* @param {string} name
* @param {!LockOptions|!LockGrantedCallback} options
* @param {!LockGrantedCallback|undefined=} callback
* @return {!Promise<void>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be either !Promise<*> or !Promise<?>, as https://www.w3.org/TR/web-locks/#api-lock-manager has this returning Promise<any>?

!Promise<*> seems safer as it forces people to typecast the result before using it, but OTOH https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts has Promise<any> which is equivalent to Promise<?>, and we're generally trying to maintain compatibility with the TS typings



/**
* @typedef {function(!Lock): !Promise<void>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, should this be !Promise<?> (or !Promise<*>)? https://www.w3.org/TR/web-locks/#api-lock-manager


/**
* @typedef {{
* mode : !LockMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding of the spec language, these should all be optional |undefined as they're not explicitly specified as required

https://webidl.spec.whatwg.org/#required-dictionary-member

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are required, each member of LockInfo has defined non-nullable type and none is marked as optional - that is explicit requirement. LockInfo itself is then specified as non-nullable and not optional type in non-nullable and not optional sequence that defines both members of LockManagerSnapshot and thus making them also explicitly required. None in this chain can be optional or even undefined:

In the JavaScript binding, a value of undefined for the property corresponding to a dictionary member is treated the same as omitting that property.

Now those sequences of LockManagerSnapshot are zero-length by default (no locks held or pending) and can be zero-length at any point in time, but even then they are not optional. So putting in all together the promise of query() method not only cannot reject, but also when it resolves it must always resolve to LockManagerSnapshot with both members present, even if they are just empty arrays.

And so it does:

Screenshot 2024-02-29 at 23 56 15


/**
* @typedef {{
* held : !Array<!LockInfo>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about |undefined as in LockInfo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants