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

lib: add weak event handlers by benjamingr · Pull Request #36607 · nodejs/node · GitHub

/ node Public

lib: add weak event handlers - #36607

Closed
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:event-target-weak-handlers
ConversationCommits (1)ChecksFiles changed
Closed

lib: add weak event handlers#36607
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:event-target-weak-handlers

Conversation

benjamingr commented Dec 22, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

benjamingr commented Dec 23, 2020

Copy link
Copy Markdown
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 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 23, 2020
benjamingr force-pushed the event-target-weak-handlers branch from f3d27aa to cc5c8bb Compare December 23, 2020 09:26
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 commented Dec 23, 2020

Copy link
Copy Markdown
Collaborator

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

benjamingr mentioned this pull request Dec 23, 2020
Closed

benjamingr commented Dec 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

benjamingr commented Dec 24, 2020
View reviewed changes
Comment thread lib/internal/event_target.js Outdated
aduh95 reviewed Dec 26, 2020
View reviewed changes
Comment thread .eslintrc.js Outdated
benjamingr added the blocked PRs that are blocked by other issues or PRs. label Dec 27, 2020

benjamingr commented Dec 27, 2020

Copy link
Copy Markdown
Member Author

I need to think about this more.

benjamingr commented Feb 4, 2021

Copy link
Copy Markdown
Member Author

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

benjamingr force-pushed the event-target-weak-handlers branch from cc5c8bb to 512ea05 Compare February 4, 2021 10:54

benjamingr commented Feb 4, 2021

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

Copy link
Copy Markdown
Collaborator

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

aduh95 reviewed Feb 4, 2021
View reviewed changes
Comment thread lib/internal/event_target.js Outdated
Comment thread lib/internal/event_target.js Outdated

aduh95 commented Feb 4, 2021

Copy link
Copy Markdown
Contributor

Do you plan to add documentation for this feature?

benjamingr mentioned this pull request Feb 4, 2021
Open

benjamingr commented Feb 4, 2021

Copy link
Copy Markdown
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:

benjamingr force-pushed the event-target-weak-handlers branch from 512ea05 to 1814443 Compare February 4, 2021 15:31

nodejs-github-bot commented Feb 4, 2021

Copy link
Copy Markdown
Collaborator

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

benjamingr commented Feb 6, 2021

Copy link
Copy Markdown
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
lib: add weak event handlers
5ef4c64 
PR-URL: #36607
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

benjamingr commented Feb 6, 2021

Copy link
Copy Markdown
Member Author

Landed in 5ef4c64 🎉

benjamingr closed this Feb 6, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
lib: add weak event handlers
acabe08 
PR-URL: #36607
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
This was referenced Feb 16, 2021
Closed
Merged
benjamingr mentioned this pull request Aug 3, 2021
Closed
debadree25 added a commit to debadree25/node that referenced this pull request Feb 4, 2023
lib: add aborted() utility function
6e797f5 
Fixes: nodejs#37220
Refs: nodejs#36607
debadree25 mentioned this pull request Feb 4, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Feb 7, 2023
lib: add aborted() utility function
6d584ae 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
lib: add aborted() utility function
8590eb4 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
lib: add aborted() utility function
2f79f3f 
Fixes: #37220
Refs: #36607
PR-URL: #46494
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@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

aduh95 aduh95 left review comments
jasnell jasnell approved these changes
addaleax Awaiting requested review from addaleax
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL