mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
535 stars 120 forks source link

fix(ClientRequest, Fetch): support miniflare by checking "Response.type" access #479

Closed mattcosta7 closed 9 months ago

mattcosta7 commented 9 months ago

quick tests/updates to support usage in miniflare/cloudflare like environmetns.

I'm not super familiar here, but this should fix and add some regression tests for miniflare environments.

The issue here was that Response.type is not implemented in miniflare, and accessing that property throws, so we need to guard against that and validate it can be accessed before attempting to access it.

Do we have a different path to validating Response.type === 'error'? would Response.status = 0 suffice, or something similar? If we do we may need additional error tests here, but checking in progress for now

Response.error() is also not defined in miniflare, so I don't think we can create a Response with a type of 'error' or similar, outside of status code checks?

relates to https://github.com/mswjs/msw/issues/1834

kettanaito commented 9 months ago

The issue here was that Response.type is not implemented in miniflare, and accessing that property throws, so we need to guard against that and validate it can be accessed before attempting to access it.

This begins to sound like an overly harsh Not Implemented behavior in Miniflare forces us into uncertainty-driven engineering here. I see no reason not to implement the "type" property on Response but this isn't my area to reason about.

Do we have a different path to validating Response.type === 'error'?

I wouldn't adjust a perfectly valid implementation to comply with environments that don't implement the Fetch API specification correctly. I don't mind a compromise of accessing the type property a bit more cautiously, but I think that's where our responsibility ends.

Response.error() is also not defined in miniflare, so I don't think we can create a Response with a type of 'error' or similar, outside of status code checks?

Then it looks like you cannot emulate network errors in Miniflare. This should be clearly documented on Miniflare's side so their users are aware of it.

kettanaito commented 9 months ago

@mattcosta7, thanks for working on this. I think I understand the issue with Miniflare better now.

I've restructured the tests, moved the XHR test into its own thing so we could assert interceptor.apply() itself, added a comment regarding the Response.type access and also a new test for mocking network errors with Response.error(). Everything is passing now.

Anything missing here? Should we merge once it's green?

kettanaito commented 9 months ago

I will go and merge this, this looks self-contained. We can always iterate on this later.

kettanaito commented 9 months ago

Released: v0.25.10 🎉

This has been released in v0.25.10!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.