Skip to content
This repository has been archived by the owner on Oct 30, 2022. It is now read-only.

Add unmock-fetch. #312

Merged
merged 1 commit into from
Oct 11, 2019
Merged

Add unmock-fetch. #312

merged 1 commit into from
Oct 11, 2019

Conversation

ksaaskil
Copy link
Contributor

@ksaaskil ksaaskil commented Oct 11, 2019

  • Adds unmock-fetch package for intercepting calls to global.fetch or window.fetch
  • Also provides buildFetch that takes in a callback function of type OnSerializedRequest
  • Used either via const fetch = buildFetch(onSerializedRequest) or with UnmockFetch.on(onSerializedRequest); ... UnmockFetch.off();. The latter patches global.fetch or window.fetch.
  • Move OnSerializedRequest to interfaces.ts in unmock-core so it can imported

TODO:

  • Get rid of hackish imports from unmock-core/dist/interfaces. Importing from unmock-core is not possible at the moment as it imports a lot of Node.js related stuff and that would break in React Native

@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev@1e07444). Click here to learn what that means.
The diff coverage is 90.62%.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev     #312   +/-   ##
======================================
  Coverage       ?   84.95%           
======================================
  Files          ?       55           
  Lines          ?     2220           
  Branches       ?      564           
======================================
  Hits           ?     1886           
  Misses         ?      334           
  Partials       ?        0
Impacted Files Coverage Δ
packages/unmock-core/src/interfaces.ts 100% <ø> (ø)
packages/unmock-core/src/backend/index.ts 84.31% <ø> (ø)
packages/unmock-fetch/src/types.ts 100% <100%> (ø)
packages/unmock-fetch/src/fetch.ts 100% <100%> (ø)
packages/unmock-fetch/src/serialize.ts 85.71% <85.71%> (ø)
packages/unmock-fetch/src/index.ts 87.5% <87.5%> (ø)

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 1e07444...4b8ec14. Read the comment docs.

@ksaaskil ksaaskil marked this pull request as ready for review October 11, 2019 10:36
Improve React Native.

Fix tests.

Fix tests more.

Fix browser export.

Add new package.

Work on fetch-mitm.

More work.

Extract algorithm part.

Rename package as unmock-fetch.

Revert changes to unmock.

More clean-up.

Fix a bug in response body.

Remove console.logs.

More clean-up.

Use types from unmock-core.

Refactor types.

Add tests, remove algorithm.

Clean-up.

Remove unused deps.

Remove unnecessary test.
Copy link
Contributor

@idantene idantene left a comment

Choose a reason for hiding this comment

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

Looks good;

Just out of curiosity - couldn't we have used cross-fetch immediately instead of fetch?

},
"dependencies": {
"@types/debug": "^4.1.5",
"@types/url-parse": "^1.4.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should debug and its @types be part of the dependencies? (and not dev dependencies?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a library should include the types it uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, but debug is not a dependency. The code doesn't need it to function properly, so shouldn't it be part of dev dependencies?

Copy link
Contributor Author

@ksaaskil ksaaskil Oct 11, 2019

Choose a reason for hiding this comment

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

But debug is a dependency of unmock-fetch :) It's true that user's type-checking probably would pass fine even if @types/debug wasn't part of the dependencies, but I think it's generally just bad practice to leave out the types. We've had two breakages in releases already because of leaving out types for sinon and node-fetch, requiring users to manually add the types.

let fetch: Fetch;
}

let mitm: Mitm | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same mitm we have in the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the Mitm is a new class defined in the same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that, but it seems you tried to maintain the same interface, hence the question. It's a bit misleading IMO to have two Mitm classes with different purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I can rename it if it makes development simpler for us. But in general users of this would not know anything about node-mitm and its API so I just picked a short name that makes it clear what it does. I can call it FetchInterceptor or something.

@ksaaskil
Copy link
Contributor Author

Hmm, I don't see what you mean by "couldn't we have used cross-fetch"? Do you mean patching crossFetch.fetch instead of patching global.fetch or window.fetch? I tried something like that but could not figure out how to actually do it.

@ksaaskil
Copy link
Contributor Author

ksaaskil commented Oct 11, 2019

Or if you mean the unmock.fetch included in the package export, it would indeed probably be better if it was indeed set to cross-fetch instead of node-fetch.

@ksaaskil ksaaskil merged commit d26272b into dev Oct 11, 2019
@ksaaskil ksaaskil deleted the fetch-mitm branch October 11, 2019 13:00
@idantene
Copy link
Contributor

Or if you mean the unmock.fetch included in the package export, it would indeed probably be better if it was indeed set to cross-fetch instead of node-fetch.

Yup, meant this :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants