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.

Sign up for GitHub

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

Jump to bottom

lib: fix WebIDL object and dictionary type conversion #37047

Merged
merged 0 commits into from
Feb 9, 2021
Merged

lib: fix WebIDL object and dictionary type conversion #37047

merged 0 commits into from
Feb 9, 2021

Conversation

ExE-Boss
Copy link
Contributor

The WebIDL object, record<K, V>, 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
@ExE-Boss ExE-Boss mentioned this pull request Jan 24, 2021
Merged
jasnell
jasnell reviewed Jan 25, 2021
View reviewed changes
lib/internal/event_target.js Outdated
@@ -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

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/928/ (queued)

@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 (***)

aduh95
aduh95 reviewed Feb 5, 2021
View reviewed changes
lib/internal/event_target.js Outdated
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

CI: https://ci.nodejs.org/job/node-test-pull-request/35958/

@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

CI: https://ci.nodejs.org/job/node-test-pull-request/35959/

@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 •
edited
Loading

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

CI: https://ci.nodejs.org/job/node-test-pull-request/36010/

@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
@ExE-Boss @danielleadams
lib: fix WebIDL object and dictionary type conversion
f6f9af6 
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
Closed
Merged
targos pushed a commit that referenced this pull request May 27, 2021
@ExE-Boss @targos
lib: fix WebIDL object and dictionary type conversion
e08fa08 
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
@ExE-Boss @targos
lib: fix WebIDL object and dictionary type conversion
4c56ac7 
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
@ExE-Boss @targos
lib: fix WebIDL object and dictionary type conversion
35c54d0 
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 targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@ExE-Boss @targos
lib: fix WebIDL object and dictionary type conversion
16691be 
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
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ExE-Boss @aduh95 @joyeecheung @nodejs-github-bot @Trott @jasnell