fix: only activate once, when on #1824
Conversation
While debugging the Jest memory leak, I was surprised to find the Nock was "activating" twice when imported. This is because intercept.js was calling `activate()` directly _AND_ back was setting the mode, which calls `restore()` and `activate()`. There is no need for this double initialization. Think of the CPUs we could be saving. I also discovered that Nock "activates" even if the proc is started with the `NOCK_OFF` env var set, which means Nock is polluting the global scope even when callers have asked it not to. Since it felt like the `back.setMode()` call was hiding in its module, I've moved it to the entry index file and wrapped it in a `isOn` check.
| // deliberate or is even helpful. This shim is included for backward | ||
| // compatibility and shoulud be replaced with an alias to `removeAll()`. | ||
| // compatibility and should be replaced with an alias to `removeAll()`. |
There was a problem hiding this comment.
Can I blame my MacBook keyboard for this? 😆Thanks for the fix.
There was a problem hiding this comment.
I'm in the same boat. Everything I need an "f" there's a 50/50 chance my laptop will explode.
|
|
||
| if (isOn()) { | ||
| // Setting the Back mode "activates" Nock by overriding the global entries in the `http/s` modules. | ||
| back.setMode(process.env.NOCK_BACK_MODE || 'dryrun') |
There was a problem hiding this comment.
I think this is doing the right thing, though this implementation seems like a smell. I suppose it's because Nock Back is a sort of separable and optional component which has nothing to do getting Nock set up, and here is used for a side effect.
I really appreciate your making this change, though wonder, is it possible to refactor this a bit further so the code here looks more like if isOn() { activate() }? Perhaps Nock Back could be lazily set up, either when it is first used or if setMode() is invoked directly.
There was a problem hiding this comment.
That would mean Back wouldn't be in "dryrun" mode when Nock is imported. What all would that break?
Also if NOCK_BACK_MODE is set, we'd still need the Back call somewhere.
There was a problem hiding this comment.
That would mean Back wouldn't be in "dryrun" mode when Nock is imported. What all would that break?
Hmm, that's a good question. Back's behavior is undefined outside of calls to nock.back() though devs are probably used to today's behavior, which is that NOCK_BACK_MODE is honored. I guess that would have to be dealt with in a breaking change.
Okay, I guess it's fine for now to do this, though leet's add to the comment an explanation that (1) we always activate nock on import, overriding the globals, (2) if nock back is configured, we need to honor that setting for backward compatibility reasons, and (3) otherwise we rely on nock back's default initializing side effect.
|
@paulmelnikow I updated the comment. This should be good to go now. |
|
I'm not sure if this is a feature. I could see shipping it as a patch, or else as a breaking change along with the other changes being made in #1871. |
mastermatt
changed the title
|
🎉 This PR is included in version 11.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
While debugging the Jest memory leak, I was surprised to find the Nock
was "activating" twice when imported. This is because intercept.js
was calling
activate()directly AND back was setting the mode, whichcalls
restore()andactivate(). There is no need for this doubleinitialization. Think of the CPUs we could be saving.
It also makes debugging harder.
I also discovered that Nock "activates" even if the proc is started with
the
NOCK_OFFenv var set, which means Nock is polluting the globalscope even when callers have asked it not to.
Since it felt like the
back.setMode()call was hiding in its module,I've moved it to the entry index file and wrapped it in a
isOncheck.