Skip to content
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: default mock interceptor to GET #1285

Merged
merged 2 commits into from Mar 27, 2022
Merged

Conversation

JustinBeckwith
Copy link
Contributor

I am in the process of migrating an open source project from node-fetch to undici. I am making heavy use of nock, which doesn't exactly work here :) One of the things I noticed is that nock tends to default to assuming GET on interceptors, and I think it's a fairly accepted norm in terms of request libraries. This change makes method optional on the MockClient.intercept method, and defaults it to a GET in that case.

const mock = new MockAgent();
const scope = mock
      .get('http://fake.local')
      .intercept({path: '/'}) // look ma, no {method: 'GET'}!
      .reply(200);

Not sure if this is a desirable API change or not, but thought it would aid in making the transition from nock to here a tad easier.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

/** Method to intercept on. */
method: string | RegExp | ((method: string) => boolean);
/** Method to intercept on. Defaults to GET. */
method?: string | RegExp | ((method: string) => boolean);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for the type? We use tsd!

@mcollina
Copy link
Member

could you also update the docs?

@JustinBeckwith
Copy link
Contributor Author

Updated docs and added a type test! Sorry for the delay.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #1285 (f57d4f3) into main (392213e) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
+ Coverage   94.11%   94.16%   +0.04%     
==========================================
  Files          44       45       +1     
  Lines        4098     4112      +14     
==========================================
+ Hits         3857     3872      +15     
+ Misses        241      240       -1     
Impacted Files Coverage Δ
lib/mock/mock-interceptor.js 100.00% <100.00%> (ø)
lib/fetch/response.js 84.86% <0.00%> (-7.56%) ⬇️
lib/pool-base.js 96.77% <0.00%> (-0.98%) ⬇️
lib/fetch/body.js 98.33% <0.00%> (-0.84%) ⬇️
lib/client.js 97.93% <0.00%> (-0.11%) ⬇️
lib/agent.js 100.00% <0.00%> (ø)
lib/proxy-agent.js 100.00% <0.00%> (ø)
lib/fetch/headers.js 100.00% <0.00%> (ø)
lib/fetch/constants.js 100.00% <0.00%> (ø)
lib/mock/mock-agent.js 100.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 392213e...f57d4f3. Read the comment docs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 48c326f into nodejs:main Mar 27, 2022
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants