fix(NODE-4281): ensure that the driver always uses Node.js timers #3275
Conversation
durran
added
the
Primary Review
|
Just for clarification, this is specifically for when the driver is used in the Electron renderer process, where the global functions are the browser functions? And we still have Node available so simply requiring 'timers' gets us the Node specific ones in that environment? |
|
@durran Yes, exactly 👍 |
durran
added
Team Review
| @@ -0,0 +1,22 @@ | |||
| 'use strict'; | |||
| const sinon = require('sinon'); | |||
|
|
|||
There was a problem hiding this comment.
I'm okay with this approach but could another approach be only linting for global setTimeout/Immediate/Intervals in the src directory? As long as Compass isn't running our tests I think that should be okay.
I don't have a strong preference, but the benefit of only linting the src directory is one fewer custom test configurations to maintain. @addaleax
There was a problem hiding this comment.
@baileympearson I’d also be okay with applying the linting only there, I guess what would mean a new .eslintrc file inside src? That should be easy enough.
That being said, this particular file would still be necessary, because it’s about how sinon mocks the timers functions (which are then picked up by files in the src/ directory)
There was a problem hiding this comment.
Oh, right. That makes sense.
If we need this file either way, I'd rather lint everything for consistency 😃
Ensure that the driver always uses the Node.js timers API, rather than the global one, in case they diverge. This affects Compass, where the `setTimeout(...).unref()` usage currently results in uncaught exceptions because `setTimeout()` returns a number in browsers. This change adds `import ... from 'timers';` where necessary and adds a linter rule to prevent regressions. If this is not an acceptable solution, we can go back to the drawing board, but this seems like a good solution that doesn’t add too much overhead when writing driver code.
addaleax
dismissed stale reviews from baileympearson, nbbeeken, and durran
via
a4dd3da
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.
Description
What is changing?
Ensure that the driver always uses the Node.js timers API, rather
than the global one, in case they diverge. This affects Compass,
where the
setTimeout(...).unref()usage currently results inuncaught exceptions because
setTimeout()returns a number inbrowsers.
This change adds
import ... from 'timers';where necessary andadds a linter rule to prevent regressions. If this is not
an acceptable solution, we can go back to the drawing board,
but this seems like a good solution that doesn’t add too much
overhead when writing driver code.
Is there new documentation needed for these changes?
No
What is the motivation for this change?
See ticket –
setTimeout().unref()doesn’t work in Compass renderersDouble check the following
npm run check:lintscript<type>(NODE-xxxx)<!>: <description>