node icon indicating copy to clipboard operation
node copied to clipboard

events: extract events file to a separate files

Open rluvaton opened this issue 1 year ago • 6 comments

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

rluvaton avatar Apr 27 '24 18:04 rluvaton

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

github-actions[bot] avatar Apr 27 '24 18:04 github-actions[bot]

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

nodejs-github-bot avatar Apr 27 '24 18:04 nodejs-github-bot

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

avivkeller avatar Apr 27 '24 19:04 avivkeller

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.

joyeecheung avatar Apr 27 '24 20:04 joyeecheung

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

targos avatar Apr 28 '24 07:04 targos

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

rluvaton avatar Apr 28 '24 10:04 rluvaton