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

fix(NODE-4444): use Node.js clear timers #3327

Merged
merged 1 commit into from Jul 19, 2022

Conversation

alecgibson
Copy link
Contributor

This change follows on from #3275
which enforced using Node.js timer methods.

However, it didn't enforce using the Node.js clear methods, which
could fall prey to similar issues.

This change adds linter rules for those clear methods, and fixes the
resulting linter errors.

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

This change follows on from mongodb#3275
which enforced using Node.js timer methods.

However, it didn't enforce using the Node.js _clear_ methods, which
could fall prey to similar issues.

This change adds linter rules for those clear methods, and fixes the
resulting linter errors.
@alecgibson
Copy link
Contributor Author

I didn't know how best to add tests for this; happy to receive guidance if you think they're necessary.

Copy link
Contributor

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I didn't know how best to add tests for this

Maybe the rest of the team has opinions here, but I think the lint rule is pretty good already.

Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, you actually beat me to it (I had a TODO in an unrelated draft PR to address exactly this :) ) - I need to double check the failures in our CI to make sure they are unrelated, but otherwise looks good to me!

@dariakp dariakp added the Team Review Needs review from team label Jul 19, 2022
@alecgibson
Copy link
Contributor Author

I need to double check the failures in our CI to make sure they are unrelated

I got as far as running npm test locally (which passes). I obviously can't see the failing build, so please let me know if there's anything I need to do (or feel free to just push on top of my commit if that's faster)!

@dariakp dariakp merged commit c5cfe21 into mongodb:main Jul 19, 2022
@dariakp
Copy link
Contributor

dariakp commented Jul 19, 2022

@alecgibson no worries, we have a known issue with our latest server version that just came in, nothing to do with this PR. Thanks again for contributing!

@alecgibson alecgibson deleted the NODE-4444/nodejs-clear-timers branch July 19, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants