Skip to content

events: set target property to null #33615

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

benjamingr
Copy link
Member

Another small fix to set the target property of the event to null and not undefined (to align behaviors)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@benjamingr benjamingr requested review from jasnell and targos May 28, 2020 14:44
@benjamingr benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 28, 2020
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2020
@benjamingr benjamingr force-pushed the event-target-target-null branch from 1dd2055 to 474f586 Compare May 30, 2020 11:30
benjamingr added a commit that referenced this pull request May 31, 2020
PR-URL: #33615
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@benjamingr
Copy link
Member Author

benjamingr commented May 31, 2020

Landed in b2b95eb 🎉

@benjamingr benjamingr closed this May 31, 2020
@benjamingr benjamingr deleted the event-target-target-null branch May 31, 2020 10:30
@targos
Copy link
Member

targos commented May 31, 2020

@benjamingr I don't know which of the PRs you just landed introduced it, but there's a linter error on master now: https://github.com/nodejs/node/runs/724700725

@benjamingr
Copy link
Member Author

@targos my bad, this is the first time I used the node core utils flow and I probably messed something up. Should I force-push a fix?

@benjamingr
Copy link
Member Author

I will force push and make a PSA, sorry!

@benjamingr
Copy link
Member Author

benjamingr commented May 31, 2020

@targos I am having issues, I am dealing with it, it will take 5 more minutes. My computer just crashed and ncu kicked me out :(

@benjamingr
Copy link
Member Author

@targos can you please check I did things correctly? I landed things for the first time in ±2 years and for the first time with NCU. Ruben helped me for most parts but I think I still made a mess somehow and I'm not sure I understand what happened.

I will just wait for Ruben to be online and then investigate with him and make a PSA in the meantime.

@BridgeAR
Copy link
Member

Seems like it's still on master. The issue is this commit f643cf4

@benjamingr benjamingr restored the event-target-target-null branch May 31, 2020 12:16
@BridgeAR BridgeAR reopened this May 31, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@benjamingr benjamingr force-pushed the event-target-target-null branch from 50ef651 to 2738e94 Compare June 5, 2020 22:57
@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Jun 5, 2020
PR-URL: #33615
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 5, 2020

Landed in 1969ada

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants