FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: fix WebIDL `object` and dictionary type conversion by ExE-Boss · Pull Request #37047 · nodejs/node · GitHub

/ node Public

lib: fix WebIDL object and dictionary type conversion - #37047

Merged
Trott merged 0 commit into
nodejs:masterfrom
ExE-Boss:lib/use-correct-webidl-object-conversion
Feb 9, 2021
ConversationCommits (0)ChecksFiles changed
Merged

lib: fix WebIDL object and dictionary type conversion#37047
Trott merged 0 commit into
nodejs:masterfrom
ExE-Boss:lib/use-correct-webidl-object-conversion

Conversation

ExE-Boss commented Jan 24, 2021

Copy link
Copy Markdown
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 added the crypto Issues and PRs related to the crypto subsystem. label Jan 24, 2021
ExE-Boss mentioned this pull request Jan 24, 2021
Merged
jasnell reviewed Jan 25, 2021
View reviewed changes
Comment thread lib/internal/event_target.js Outdated

jasnell Jan 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aduh95 commented Jan 29, 2021

Copy link
Copy Markdown
Contributor

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

joyeecheung commented Feb 1, 2021

Copy link
Copy Markdown
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 commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

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

aduh95 commented Feb 5, 2021

Copy link
Copy Markdown
Contributor

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 reviewed Feb 5, 2021
View reviewed changes
Comment thread lib/internal/event_target.js Outdated
Comment on lines 88 to 90

aduh95 Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ExE-Boss Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 commented Feb 5, 2021

Copy link
Copy Markdown
Collaborator

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

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

nodejs-github-bot commented Feb 5, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. review wanted PRs that need reviews. labels Feb 5, 2021

ExE-Boss commented Feb 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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 commented Feb 8, 2021

Copy link
Copy Markdown
Collaborator

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

Trott commented Feb 9, 2021

Copy link
Copy Markdown
Member

Landed in beee538

Trott closed this Feb 9, 2021
Trott force-pushed the lib/use-correct-webidl-object-conversion branch from b6eb2bd to beee538 Compare February 9, 2021 01:03
Trott merged commit beee538 into nodejs:master Feb 9, 2021
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
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
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
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
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 mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
joyeecheung joyeecheung approved these changes
aduh95 aduh95 approved these changes
jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL