Skip to content

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

Development

Successfully merging this pull request may close these issues.

3 participants