-
-
Notifications
You must be signed in to change notification settings - Fork 133
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(ClientRequest, Fetch): support miniflare by checking "Response.type" access #479
Conversation
name: 'setup-server', | ||
interceptors: [ | ||
new ClientRequestInterceptor(), | ||
new XMLHttpRequestInterceptor(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XMLHttp is not defined in miniflare, but setupServer
in msw defaults to applying it. it's a noop, but I've kept that here, even though it's unused - as a regression check that we can apply it
We may want to look into scoping an interceptor set for that environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I read about this, the more all of this becomes the limitation of Miniflare. You can create a custom setup*
function with a subset of interceptors even as of now, no changes needed:
import { SetupServerApi } from 'msw/node'
import { ClientRequestInterceptor } from '@mswjs/interceptors/ClientRequest'
import { FetchInterceptor } from '@mswjs/interceptors/fetch'
function setupMiniflareServer(...handlers) {
return new SetupServerApi([
new ClientRequestInterceptor(),
new FetchInterceptor()
], ...handlers)
}
Then, in your code, instead of setupServer
you call setupMiniflareServer
:
setupMiniflareServer(...yourHandlersHere)
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
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
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. |
test/third-party/miniflare.test.ts
Outdated
test('Some response properties/methods throw', async () => { | ||
// If this test fails in the future, we can remove the property access | ||
// safety checks in the interceptors. | ||
expect(() => new Response('body').type).toThrow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is something we should be testing. These tests don't even involve Interceptors so they assert the behavior of a third-party (vitest-environment-miniflare
in this case). This is incorrect.
Instead, if we wish to assert what happens on Response.type
access, create a request listener that responds with that call. It will throw, and we will receive a request error, I believe. Assert that.
test/third-party/miniflare.test.ts
Outdated
* issues with miniflare. | ||
*/ | ||
test('XMLHttpRequest is not available', () => { | ||
expect(() => XMLHttpRequest).toThrow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment here: the fact that XMLHttpRequest
is unavailable in Miniflare isn't related in any way to Interceptors. We should drop this test.
@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 Anything missing here? Should we merge once it's green? |
I will go and merge this, this looks self-contained. We can always iterate on this later. |
Co-authored-by: Artem Zakharchenko <[email protected]>
Released: v0.25.10 🎉This has been released in v0.25.10! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Response.type
throws in miniflare-like environments #478quick 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 nowResponse.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 mswjs/msw#1834