- Notifications
You must be signed in to change notification settings - Fork3.5k
Add assertions to emscriptenGetAudioObject. NFC#25914
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
8e311af to587a47fCompare587a47f to60e5204Compare| $emscriptenGetAudioObject:(objectHandle)=>{ | ||
| #ifASSERTIONS||WEBAUDIO_DEBUG | ||
| emAudioExpectNodeOrContext(objectHandle,'emscriptenGetAudioObject'); | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The above call toemAudioExpectNodeOrContext() seems restrictive.
It prevents users from testing if a given handle has been deleted? E.g. one would not be able to write code like this anymore:
if (emscriptenGetAudioObject(someHandle)) { console.log('someHandle exists, accessing it...');} else { console.log('someHandle has been deleted, doing something else (maybe deinitializing some JS side structure at shutdown?)');}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, that is the idea. None of our use cases for the API seems to use it in this way, and none of the other APIs in the this file allow invalid handles to be passed. Unless there is specific use case for allowing invalid handles here I suggest we be consistent. (At least until someone asks for this ability).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd be interested to know how others are using the API. I agree with putting the test with the handle request, but I only see my own use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm only aware of the uses within the test code, but maybe you could find more via a github global search or some such?
Uh oh!
There was an error while loading.Please reload this page.
This API was added when audioworklets were first added, but it doesn't
look like it was intended to work with invalid inputs:5402fc9.
At least none of the original usages in5402fc9 checked for