Skip to content

Refactor - IDL class def #104

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

Merged
merged 11 commits into from
Apr 3, 2024
Merged

Refactor - IDL class def #104

merged 11 commits into from
Apr 3, 2024

Conversation

b-ma
Copy link
Collaborator

@b-ma b-ma commented Apr 3, 2024

Some heavy work and JS black magic to make more idlharness tests pass

$ npm run wpt:only -- --filter idlharness
RESULTS (before):
  - # pass: 632
  - # fail: 511
  - # type error issues: 0
  
RESULTS (after):
  - # pass: 871
  - # fail: 272
  - # type error issues: 0

Impact on overall wpt is interesting too (unlocked quite a few tests :)

$ npm run wpt
RESULTS (before):
  - # pass: 6240
  - # fail: 1072
  - # type error issues: 24

RESULTS (after):
  - # pass: 6739
  - # fail: 800
  - # type error issues: 24

@b-ma b-ma requested a review from orottier April 3, 2024 08:35
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.

Alright, I don't understand everything but the results are solid!

@@ -130,6 +130,10 @@ module.exports = (jsExport, nativeBinding) => {
}

set fftSize(value) {
if (!(this instanceof AnalyserNode)) {
Copy link
Collaborator

@orottier orottier Apr 3, 2024

Choose a reason for hiding this comment

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

I'm very surprised we need to check this inside the class AnalyserNode?
Also, instanceof works with inheritance properly right? So if a user has MyNode extends AnalyserNode these methods are still callable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yup I was a bit surprised too at first... but actually it makes sens since this is always possible to try to execute anything with the this you want in JS, e.g. this is the test which is made in the idlharness (warning it can hurt you eyes :):

const desc = Object.getOwnPropertyDescriptor(AnalyserNode.prototype, 'fftSize');
desc.set.call({}, 256);

Also, instanceof works with inheritance properly right. So if a user has MyNode extends AnalyserNode these methods are still callable?

Yup it should, which is nice

@b-ma b-ma merged commit 0fcec9e into main Apr 3, 2024
5 checks passed
@b-ma b-ma deleted the refactor/idl-class-def branch April 3, 2024 17:12
@b-ma b-ma mentioned this pull request Apr 3, 2024
12 tasks
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