Skip to content

Comments

esm: protect against removal of "beforeExit" event handler#47967

Closed
aduh95 wants to merge 1 commit intonodejs:mainfrom
aduh95:beforeExit-loader-thread
Closed

esm: protect against removal of "beforeExit" event handler#47967
aduh95 wants to merge 1 commit intonodejs:mainfrom
aduh95:beforeExit-loader-thread

Conversation

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 11, 2023

tbh I'm not sure if that's a good idea, it's still quite brittle because hooks authors could still remove the removeListener listener (e.g. by calling process.removeAllListeners()), and at some point there's no way around it – or at least, I didn't find a way. I'm opening this in case someone has a better solution they'd want to share.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels May 11, 2023
@GeoffreyBooth
Copy link
Member

What problem is this solving?

If you’re intending to prevent them from removing the event handler, yet they still can, how is this an improvement? Yes they need to make a different call to do so, but if someone is trying to break things and they still are able to, I’m not sure we’ve achieved anything.

@aduh95
Copy link
Contributor Author

aduh95 commented May 11, 2023

@GeoffreyBooth did you miss the "I'm opening this in case someone has a better solution they'd want to share" part of the OP?

@GeoffreyBooth
Copy link
Member

@GeoffreyBooth did you miss the β€œI’m opening this in case someone has a better solution they’d want to share” part of the OP?

No, I didn’t.

@ljharb
Copy link
Member

ljharb commented May 12, 2023

… wait, why is removeAllListeners a thing? The web doesn't have this precisely because it's an encapsulation violation.

@targos
Copy link
Member

targos commented May 12, 2023

@ljharb It's been in Node.js since version 0.1.32: f8eb163

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 29, 2024

I guess we can triage this in "known limitations"

@aduh95 aduh95 closed this Jan 29, 2024
@aduh95 aduh95 deleted the beforeExit-loader-thread branch January 29, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants