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

Implement EventTarget interface #61

Merged
merged 15 commits into from
Jan 2, 2024
Merged

Implement EventTarget interface #61

merged 15 commits into from
Jan 2, 2024

Conversation

b-ma
Copy link
Collaborator

@b-ma b-ma commented Dec 29, 2023

Just another try compared to #52

Tried to follow your idea, seems a bit better to me indeed but there are still some convoluted things I didn't find how to work around (no big deal though, it works... :)

Note that it depends on 415 (merged)

List of all events in the spec:

todo later

  • AudioRenderCapacity::update (not wrapped yet)
  • AudioWorkletNode::processorerror (not wrapped yet)
  • ScriptProcessorNode::audioprocess (not implemented yet)

@b-ma
Copy link
Collaborator Author

b-ma commented Dec 29, 2023

One thing that I found that is more a concern on the rust side, is that the first "resume" statechange event is sometimes triggered, sometimes not. I think this some kind of race condition but this is a bit weird from a user perspective.

I will try to make a failing test for that later

Copy link
Collaborator

@orottier orottier left a comment

Choose a reason for hiding this comment

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

This all looks quite complicated but I guess this is due to the fact that NodeJs has no JS-alike event targets?

// We need to clean things around so that the js object can be garbage collected.
// But we also need to wait so that the previous tsfn.call is executed,
// this is not clean, but don't see how to implement that properly right now.
std::thread::sleep(std::time::Duration::from_millis(100));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope this hack is only needed for the onStateChange event? Or also for other types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hum no it seems to be required for other types as well, need to investigate further

@orottier
Copy link
Collaborator

One thing that I found that is more a concern on the rust side, is that the first "resume" statechange event is sometimes triggered, sometimes not. I think this some kind of race condition but this is a bit weird from a user perspective.

I could make a start with orottier/web-audio-api-rs#410 where the whole thing will be rewritten anyway

@b-ma
Copy link
Collaborator Author

b-ma commented Dec 30, 2023

This all looks quite complicated but I guess this is due to the fact that NodeJs has no JS-alike event targets?

Actually, this is quite boring because there is a compliant EventTarget implementation in Node, but then we would need multiple inheritance to inherit from both EventTarget and the NAPI classes, which is not possible in JS.

A solution is described here, but this is something internal to Node and the given "trick" is not exposed... so we have work around somehow... Maybe I could try to open an issue on the Node repo to see if there is a solution I didn't find

@b-ma b-ma changed the title Event emitter - new try implement EventTarget interface Dec 31, 2023
@b-ma b-ma changed the title implement EventTarget interface Implement EventTarget interface Dec 31, 2023
@b-ma
Copy link
Collaborator Author

b-ma commented Jan 1, 2024

I don't really understand why I have less tests working than you... but that was already the case before

  RESULTS:
  - # pass: 3304 (-107)
  - # fail: 1251 (-103)
  - # type error issues: 214 (-21)

Could you run the wpt see what you get?

In any case, there is already a lot (too much) done in this PR, I will merge once orottier/web-audio-api-rs#415 is released if ok for you

@orottier
Copy link
Collaborator

orottier commented Jan 2, 2024

I don't really understand why I have less tests working than you... but that was already the case before

The wpt submodule is out of date on your end I think, and this was accidentally merged into main over at fd4d710

I will push the correct version and I think you should then run git submodule update --remote --merge

I now notice we have a fatal wpt crash at

node:internal/process/promises:289
            triggerUncaughtException(err, true /* fromPromise */);
            ^
Error {
  message: 'assert_equals: state on suspend fulfilled. expected "suspended" but got "running"',
  stack: '    at http://127.0.0.1:53923/webaudio/the-audio-api/the-audiocontext-interface/suspend-after-construct.html:53:5\n' +
    '    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)'
}

So I don't have the results right now, will look into it later today

@orottier
Copy link
Collaborator

orottier commented Jan 2, 2024

In any case, there is already a lot (too much) done in this PR, I will merge once orottier/web-audio-api-rs#415 is released if ok for you

I have just released v0.40.0 for you

@b-ma
Copy link
Collaborator Author

b-ma commented Jan 2, 2024

Thanks!

Seems that it was the wpt that was not up to date indeed.

For the error you encounter, I don't really understand this works well on my side. But I have seen yesterday that building in debug mode could create weird errors and crashes with wpt, I updated the wpt script (npm run wpt) so that it build in release mode, a bit longer but no strange issues of that kind, and we can always run npm run wpt:only to bypass the build step if needed.

Updated results on my side are:

  RESULTS:
  - # pass: 3605 (+192)
  - # fail: 1284 (-69)
  - # type error issues: 234 (-1)

@b-ma b-ma merged commit dc9e602 into main Jan 2, 2024
5 checks passed
@b-ma b-ma mentioned this pull request Jan 2, 2024
12 tasks
@b-ma b-ma deleted the feature/events branch January 2, 2024 15:43
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.

None yet

2 participants