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: add weak event handlers #36607

Closed
wants to merge 1 commit into from
Closed

lib: add weak event handlers #36607

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 22, 2020 •
edited
Loading

This is a first attempt at "weak listeners".

Still a draft since I have no idea how to test this :] Do we have any idea how to test WeakRefs? EDIT: added a test.

Also still playing with this - but I think this is generally very useful and I will move certain APIs to it.

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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 22, 2020
@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 22, 2020
@benjamingr benjamingr marked this pull request as ready for review December 22, 2020 22:25
@benjamingr
Copy link
Member Author

The motivation for this is:

  • Improve the retention of handlers if a signal argument if a signal is passed to addEventListener.
  • Eventually migrate all (most?) the places that do addEventListener for abort.
  • Upstream this as .follow (or a different solution) to the DOM specification so that users (and not just core) can use it.

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr benjamingr mentioned this pull request Dec 23, 2020
Closed
@benjamingr
Copy link
Member Author

benjamingr commented Dec 24, 2020 •
edited
Loading

@jasnell @addaleax any chance this could get a review :]?

aduh95
aduh95 reviewed Dec 26, 2020
View reviewed changes
.eslintrc.js Outdated Show resolved Hide resolved
@benjamingr benjamingr added the blocked PRs that are blocked by other issues or PRs. label Dec 27, 2020
@benjamingr
Copy link
Member Author

I need to think about this more.

@benjamingr
Copy link
Member Author

Re-applied the changes with weak event handlers based on the comment (using an object as the retainer key)

@benjamingr
Copy link
Member Author

Pinging @jasnell @addaleax @nodejs/events to make sure this doesn't land without a chance to object since the design changed (very so slightly though)

@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 reviewed Feb 4, 2021
View reviewed changes
lib/internal/event_target.js Outdated Show resolved Hide resolved
lib/internal/event_target.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Feb 4, 2021

Do you plan to add documentation for this feature?

@benjamingr benjamingr mentioned this pull request Feb 4, 2021
Open
@benjamingr
Copy link
Member Author

@aduh95

Do you plan to add documentation for this feature?

Not really since this feature is internal to core - users cannot create weak listeners. This is because that is a spec requirement (we cannot add things to the standard EventTarget).

In the future - I'd like to use the mechanics of this in several utilities (and internally) namely:

@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member Author

@jasnell @addaleax @aduh95 I would really prefer not to land this without a review of the latest changes ^^

A review would be appreciated (the main change I want a LGTM on is passing a retainer object and not a boolean since that didn't work)

benjamingr added a commit that referenced this pull request Feb 6, 2021
@benjamingr
lib: add weak event handlers
5ef4c64 
PR-URL: #36607
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
@benjamingr
Copy link
Member Author

Landed in 5ef4c64 🎉

@benjamingr benjamingr closed this Feb 6, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@benjamingr @danielleadams
lib: add weak event handlers
acabe08 
PR-URL: #36607
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
This was referenced Feb 16, 2021
Closed
Merged
@benjamingr benjamingr mentioned this pull request Aug 3, 2021
Closed
debadree25 added a commit to debadree25/node that referenced this pull request Feb 4, 2023
@debadree25
lib: add aborted() utility function
6e797f5 
Fixes: nodejs#37220
Refs: nodejs#36607
@debadree25 debadree25 mentioned this pull request Feb 4, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Feb 7, 2023
@debadree25
lib: add aborted() utility function
6d584ae 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@debadree25 @MylesBorins
lib: add aborted() utility function
8590eb4 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@debadree25 @danielleadams
lib: add aborted() utility function
2f79f3f 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@devsnek devsnek devsnek approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. events Issues and PRs related to the events subsystem / EventEmitter. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

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