After starting the work to fix #2353, I found another, related issue: if scope._persist is truthy, the interceptor would not be removed from scope.keyedInterceptors but it would still be removed from allInterceptors[baseUrl].interceptors. Let me know if you prefer to have that fix it in a separate PR. It's two commits, so it's easy to split out.

Hey I realized after opening this PR that the commit messages don't follow the proper convention. I don't know if you squash PRs when merging of if you rebase them, so please let me know if you need me to rewrite the commit messages and force-push.

After starting the work to fix #2353, I found another, related issue: if scope._persist is truthy, the interceptor would not be removed from scope.keyedInterceptors but it would still be removed from allInterceptors[baseUrl].interceptors. Let me know if you prefer to have that fix it in a separate PR. It's two commits, so it's easy to split out.

@mikicho Before I put any additional effort on rebasing this work, could you please let me know if you are OK with taking both fixes in a single PR (this one) or it you want me to open a separate PR to fix Scope#remove when _persist is set?

@mbargiel Thanks! Does this break current behavior?

Hi @mikicho!

I don't think it breaks any existing, correct behaviour. It should just fix problematic cases. As I mentioned in the PR description:

Removing an interceptor with nock.removeInterceptor did not remove it from interceptor.scope.interceptors.

This means nock.removeInterceptor resulted in inconsistent scope state.

And if interceptor.scope._persist is truthy, it also did not remove it from interceptor.scope.keyedInterceptors either.

This means that when Scope#_persist is truthy, Scope#remove would leave the scope in an inconsistent state as well.

Also, there was an inconsistency that arose from Scope#add and Scope#intercept each adding an interceptor to a different collection (this.interceptors and this.keyedInterceptors), which I believe should always be synchronized. The only way to guarantee they are always synchronized is to ensure they are both modified consistently by the same function call - in this case add made more sense, since remove is the symmetric point where they are removed.

This might be a mistake on my part if this.interceptors was somehow supposed to be a lazy cache of sorts, but that didn't feel like it. As a bonus of this change, this.interceptors will now be guaranteed to be sorted in the order of interceptor registration rather than in the order of interceptions. I don't know if this matters or not but it feels more deterministic (determinism is good!) and conceptually simpler/more in line with implicit expectations.

BTW I noticed all my PRs had code formatting issues. Have you considered adding husky and lint-staged to your package automation in order to automatically run prettier in a pre-commit hook? I could share my own configuration if you'd be interested (in a separate PR/issue of course). This would ensure that all contributors automatically have this set up, and running git commit would include an automatic call to prettier to re-format the staged files (important: this only touches staged changes, which is awesome).

@mbargiel It looks good in general.
I'm a bit concerned about the behavior changes in the remove persisted interceptor, which may break some users.

I am wondering if we need to include this in the beta and release this as a breaking change.

I am wondering if we need to include this in the beta and release this as a breaking change.

when in doubt, release it as part of the beta

Sure, I have no problem with that. I see now that it's indeed an API change, so it may be prudent to not include it directly in the next non-beta release, if that's your wish.

Just to explain why I believe this is a good change: personally, I think that scope.remove(key, scope.interceptors[0]) not removing the interceptor is very awkward and a violation of the principle of least surprise. (In this case: one can reasonably think that calling the opposite method of add, remove, would effectively remove the entry from the container...) I understood that an interceptor with persist used meant it is not removed when it intercepts a call, so it can intercept indefinitely; I did not understand that it made it unremovable from the scope, and that was a nasty surprise for us. It meant that you cannot share a common scope for a series of tests if you use persist in any one test, because that scope is then forever polluted with a sticky interceptor that won't go away even if you try to remove it; you are forced to trash the whole scope, create a new one and configure it.

I see your point.

1. I'm wondering why these methods are not documented.
2. Why do we need scope.remove at all if we have nock.removeInterceptor.

Is there a chance scope.remove/add was exposed by mistake? (we use them in removeInterceptor)
@gr2m @Uzlopak, maybe you can shed some light on this.

Is there a chance scope.remove/add was exposed by mistake? (we use them in removeInterceptor)
@gr2m @Uzlopak, maybe you can shed some light on this.

I don't know, might have been before I joined @nock? I'm okay removing access in beta and see if someone complaints, then we'll know 😁

@gr2m If we decide to remove them. i suggest deprecate them for 14 and remove in 15.

Closing PR for now because it's just noise for everyone involved. I'm no longer working on projects using nock so I unfortunately don't have time to fix the PR. If you wish to fix #2353, feel free to re-open/spin up a new PR based on the proposed changes.