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: make nock.removeInterceptor
ignore scope._persist
and update scope.interceptors
#2355
base: main
Are you sure you want to change the base?
fix: make nock.removeInterceptor
ignore scope._persist
and update scope.interceptors
#2355
Conversation
scope._persist
and update scope.interceptors
nock.removeInterceptor
to ignore scope._persist
and update scope.interceptors
After starting the work to fix #2353, I found another, related issue: if |
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. |
nock.removeInterceptor
to ignore scope._persist
and update scope.interceptors
nock.removeInterceptor
ignore scope._persist
and update scope.interceptors
@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 |
Note: for consistency between scope.interceptors and scope.keyedInterceptors, interceptors are now only pushed to scope.interceptors when they are added, not when they are created. That guarantees both collections are consistent.
d4ac1cf
to
1e57041
Compare
@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:
This means
This means that when |
Also, there was an inconsistency that arose from This might be a mistake on my part if |
BTW I noticed all my PRs had code formatting issues. Have you considered adding |
@mbargiel It looks good in general. I am wondering if we need to include this in the |
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 |
@gr2m If we decide to remove them. i suggest deprecate them for 14 and remove in 15. |
Removing an interceptor with
nock.removeInterceptor
did not remove it frominterceptor.scope.interceptors
. And ifinterceptor.scope._persist
is truthy, it also did not remove it frominterceptor.scope.keyedInterceptors
either.Changes included in this PR:
Scope#remove
also remove interceptors fromscope.interceptors
, but only when the key is found.Scope#intercept
no longer adds interceptor tothis.interceptors
. That's done byScope#add
so thatthis.interceptors
andthis.keyedInterceptors
are consistent and contain the same interceptors.Other change directly related to the above, but found while adding tests
Scope#remove
no longer checksthis._persist
. The function is called from two places:Interceptor#markConsumed
, which also checksscope._persist
before callingScope#remove
nock.removeInterceptor
, where it looks like a bug because it removes the interceptor fromallInterceptors[baseUrl].interceptors
before callingScope#remove
, which then does not remove the interceptor, leading to an inconsistent state.Fixes #2353