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: only activate once, when on #1824
Changes from 1 commit
8261fbb
6a03cb1
4361506
dace878
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ const { | |
activate, | ||
isActive, | ||
isDone, | ||
isOn, | ||
pendingMocks, | ||
activeMocks, | ||
removeInterceptor, | ||
|
@@ -29,9 +30,9 @@ Object.assign(module.exports, { | |
disableNetConnect, | ||
enableNetConnect, | ||
// TODO-12.x Historically `nock.cleanAll()` has returned the nock global. | ||
// The other global methods do noto do this, so it's not clear this was | ||
// The other global methods do not do this, so it's not clear this was | ||
// 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()`. | ||
cleanAll() { | ||
removeAll() | ||
return module.exports | ||
|
@@ -49,3 +50,8 @@ Object.assign(module.exports, { | |
restore: recorder.restore, | ||
back, | ||
}) | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would mean Back wouldn't be in "dryrun" mode when Nock is imported. What all would that break? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm, that's a good question. Back's behavior is undefined outside of calls to 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Omg leet's |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I blame my MacBook keyboard for this? 😆Thanks for the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in the same boat. Everything I need an "f" there's a 50/50 chance my laptop will explode.