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

feat(scripting/v8): allow to set the max listeners count #2396

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

joelwurtz
Copy link
Contributor

Goal of this PR

By default any event that is listened more than 10 times will trigger a log error saying that there may be a memory leak.

In some cases users may want to listen more than 10 times to a specific event this patch allow to not display a log message in those cases by setting the correct value to the setMaxEventListeners.

It's not a real problem, but often users will print this error saying it's the cause of a bug (but it's not) and it's misleading.

How is this PR achieving the goal

This PR allow to configure the max listeners count so a developer can set it according to his code behavior.

This PR applies to the following area(s)

FiveM, RedM, Server, ScRT: JS

Successfully tested on

Platforms: Windows, Linux

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

N/A

@nihonium-cfx nihonium-cfx self-requested a review February 20, 2024 17:32
Copy link
Contributor

@nihonium-cfx nihonium-cfx left a comment

Choose a reason for hiding this comment

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

Hey there, thanks for the PR!

  1. Commit message needs to refer to scripting/v8 instead of scripting-v8, see this commit for example: 01a29ec
  2. TypeScript definitions need to be extended to reflect new API added.

Not required, but as a side-quest: would be great to add this functionality for the raw event listener (rawEmitter) as well for feature parity of these.

…d raw event

By default any event that is listened more than 10 times will trigger a log error saying that there may be a memory leak. In some cases users may want to listen more than 10 times to a specific event this patch allow to not display a log message in those cases by setting the correct value to the setMaxEventListeners
@joelwurtz
Copy link
Contributor Author

Thanks for the review, i have applied the feedback

@joelwurtz joelwurtz changed the title feat(scripting-v8): allow to set the max listeners count feat(scripting/v8): allow to set the max listeners count Feb 22, 2024
@nihonium-cfx nihonium-cfx self-requested a review February 23, 2024 08:23
@nihonium-cfx nihonium-cfx added the ready-to-merge This PR is enqueued for merging label Feb 23, 2024
@thorium-cfx thorium-cfx added the ScRT: JS Issues/PRs related to the JavaScript scripting runtime label Feb 23, 2024
@thorium-cfx thorium-cfx merged commit 5f7e903 into citizenfx:master Feb 23, 2024
@joelwurtz joelwurtz deleted the patch-1 branch February 23, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR is enqueued for merging ScRT: JS Issues/PRs related to the JavaScript scripting runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants