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

events: EventTarget use validators and small fix #33663

Closed
wants to merge 2 commits into from
Closed

events: EventTarget use validators and small fix #33663

wants to merge 2 commits into from

Conversation

lundibundi
Copy link
Member

  • events: fix depth in customInspectSymbol and clean up
  • events: use internal/validators in event_target.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

/cc @jasnell @nodejs/events

lundibundi
lundibundi commented May 30, 2020
View reviewed changes
lib/internal/event_target.js
@@ -66,7 +67,7 @@ class Event {
return name;

const opts = Object.assign({}, options, {
dept: options.depth === null ? null : options.depth - 1
depth: NumberIsInteger(options.depth) ? options.depth - 1 : options.depth
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd assume this was a bug (dept instead of depth) but wanted to point out since I'm not sure how this interaction works (inspecting circular events I assume?).
Also, use NumberIsInteger instead of just null check.

Copy link
Member

Choose a reason for hiding this comment

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

That is both correct. The user should only ever pass through null or and integer but it would be possible to pass through other types as well.

targos
targos reviewed May 31, 2020
View reviewed changes
lib/internal/event_target.js Outdated
Object,
NumberIsInteger,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Object,
NumberIsInteger,
NumberIsInteger,
Object,

@benjamingr
Copy link
Member

Generally LGTM - can you add a test?

@lundibundi
Copy link
Member Author

Generally LGTM - can you add a test?

Do you mean test for inspecting of Event and EventTarget with depth parameter?
I'm not sure how to do that, the Event doesn't have any circular references in customInspectSymbol handler and the EventTarget just inspects empty object (bug possibly?) in customInspectSymbol which is why I was confused by the actual need of the custom inspect there.

lundibundi
lundibundi commented May 31, 2020
View reviewed changes
lib/internal/event_target.js
@@ -73,7 +73,7 @@ class Event {
return name;
Copy link
Member Author

Choose a reason for hiding this comment

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

Also, @BridgeAR shouldn't this return this? (as well as the other one)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be ideal. this will just let inspect() to properly know what to do.

@benjamingr
Copy link
Member

Do you mean test for inspecting of Event and EventTarget with depth parameter?

Yes. cc @jasnell wdyt?

If this isn't too much work to test I think we should add a test - but if it's a lot of work I am fine with LGTMing without one.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@lundibundi
Copy link
Member Author

This is now (after force-push) waiting (adds the same code as)
#33611
#33623
#33613

@lundibundi lundibundi added blocked PRs that are blocked by other issues or PRs. events Issues and PRs related to the events subsystem / EventEmitter. labels May 31, 2020
BridgeAR
BridgeAR reviewed May 31, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This needs a rebase.

lib/internal/event_target.js
@@ -73,7 +73,7 @@ class Event {
return name;
Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be ideal. this will just let inspect() to properly know what to do.

lib/internal/event_target.js
@@ -66,7 +67,7 @@ class Event {
return name;

const opts = Object.assign({}, options, {
dept: options.depth === null ? null : options.depth - 1
depth: NumberIsInteger(options.depth) ? options.depth - 1 : options.depth
Copy link
Member

Choose a reason for hiding this comment

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

That is both correct. The user should only ever pass through null or and integer but it would be possible to pass through other types as well.

@BridgeAR BridgeAR force-pushed the validators-in-event-target branch from c434ce6 to 9a50794 Compare May 31, 2020 12:51
@BridgeAR
Copy link
Member

@lundibundi I rebased your branch and everything should be in order again. The tests pass locally. Please have a look to verify that, thanks.

@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label May 31, 2020
@lundibundi
Copy link
Member Author

@BridgeAR thanks :).
I wanted to wait until those PRs land to incorporate changes but I guess I can now just comment there instead of waiting.

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Jun 17, 2020

This needs another rebase, unfortunately.

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell jasnell added the eventtarget Issues and PRs related to the EventTarget implementation. label Jun 19, 2020
@jasnell jasnell mentioned this pull request Jun 22, 2020
Closed
3 tasks
@jasnell
Copy link
Member

jasnell commented Jun 22, 2020

Closing in favor of #34015 (which combines this and other eventtarget PRs into a single to make landing easier

@jasnell jasnell closed this Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lundibundi @benjamingr @BridgeAR @nodejs-github-bot @jasnell @targos