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

lib: fix WebIDL object and dictionary type conversion #37047

Merged

Conversation

ExE-Boss
Copy link
Contributor

The WebIDL object, record<KV>, and Dictionary type conversion algorithm implicitly allows Arrays and Functions:


This is also necessary to prevent #37028 from being a breaking change.

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jan 24, 2021
@@ -86,7 +86,9 @@ class Event {
if (arguments.length === 0)
throw new ERR_MISSING_ARGS('type');
if (options !== null)
validateObject(options, 'options');
validateObject(options, 'options', {
Copy link
Member

Choose a reason for hiding this comment

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

For performance reasons, It's likely better to introduce a separate validator function for this.

@aduh95
Copy link
Contributor

aduh95 commented Jan 29, 2021

So, should we run a benchmark CI for this, and which one?

@joyeecheung
Copy link
Member

So, should we run a benchmark CI for this, and which one?

I think the benchmark/events/eventtarget.js is one you are looking for?

@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2021

@aduh95
Copy link
Contributor

aduh95 commented Feb 5, 2021

Benchmark results are OK:

                                            confidence improvement accuracy (*)   (**)  (***)
events/eventtarget.jslisteners=10 n=1000000                -1.05 %       ±4.82% ±6.42% ±8.37%
events/eventtarget.jslisteners=1 n=1000000                 -1.17 %       ±2.20% ±2.93% ±3.83%
events/eventtarget.jslisteners=5 n=1000000                  0.38 %       ±4.03% ±5.40% ±7.11%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 3 comparisons, you can thus
expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Comment on lines 88 to 90
if (options !== null)
validateObject(options, 'options');
validateObject(options, 'options', {
allowArray: true, allowFunction: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you pass nullable: true instead of having a if (options !== null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this became possible when #35806 made options default to null.

@ExE-Boss ExE-Boss force-pushed the lib/use-correct-webidl-object-conversion branch from 9ee498a to b6eb2bd Compare February 5, 2021 12:01
@nodejs-github-bot
Copy link
Collaborator

@ExE-Boss ExE-Boss force-pushed the lib/use-correct-webidl-object-conversion branch from 4762372 to b6eb2bd Compare February 5, 2021 12:47
@ExE-Boss ExE-Boss requested a review from jasnell February 5, 2021 13:03
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Feb 5, 2021
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 6, 2021

The PR labels should probably include the events label or the eventtarget label.

@aduh95 aduh95 added events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. labels Feb 6, 2021
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 9, 2021

Landed in beee538

@Trott Trott closed this Feb 9, 2021
@Trott Trott force-pushed the lib/use-correct-webidl-object-conversion branch from b6eb2bd to beee538 Compare February 9, 2021 01:03
@Trott Trott merged commit beee538 into nodejs:master Feb 9, 2021
@ExE-Boss ExE-Boss deleted the lib/use-correct-webidl-object-conversion branch February 9, 2021 02:49
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
PR-URL: #37047
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Feb 16, 2021
targos pushed a commit that referenced this pull request May 27, 2021
PR-URL: #37047
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #37047
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
PR-URL: #37047
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
PR-URL: #37047
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants