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.

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

events: extract events file to a separate files #52726

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Apr 27, 2024

The reason I did this is that I want to create another PR to add a fast event emitter for the common cases and the events file is already large enough, if you want I can extract this PR to multiple PRs one for each file

Before merging needs git squash to avoid having commits in main that not working by themself

I extracted the following:

  1. EventEmitter to lib/internal/events/event_emitter.js
  2. on, once, and getEventListeners to internal/events/event_emitter_helpers.js
  3. EventEmitterAsyncResource to lib/internal/events/event_emitter_async_resource.js

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. labels Apr 27, 2024
@rluvaton rluvaton force-pushed the move-event-emitter-async-resource-code-to-own-file branch from b702d6b to 40e3fe7 Compare April 27, 2024 18:02
@rluvaton rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 27, 2024
Copy link
Contributor

Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/8861715452

@rluvaton rluvaton changed the title events: extract EventEmitterAsyncResource to a separate file events: extract events file to a separate files Apr 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@rluvaton rluvaton removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 27, 2024
@RedYetiDev
Copy link
Member

I'm a fan of this idea, but IMO the file names could drop the event_emitter_<...> prefix, but that's only my opinion.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 27, 2024

This would penalize startup, make back porting harder, and make git blame and git log less useful. Unless it’s getting completely rewritten I would suggest avoid moving files for moving files’ sake.

@targos
Copy link
Member

targos commented Apr 28, 2024

It's not for moving files' sake if the goal is to open another PR later that uses the extracted internals.

@rluvaton
Copy link
Member Author

I'm working on another PR that creates a fast and slow EventEmitter after talking to @benjamingr and currently the events file is really hard to work with

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM assuming this is to facilitate a refactor

@rluvaton rluvaton mentioned this pull request May 1, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants