-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
Fallback mode #688
Fallback mode #688
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d8e1652:
|
107da9e
to
b98798d
Compare
emitter: StrictEventEmitter<ServerLifecycleEventsMap>, | ||
handleRequestOptions?: HandleRequestOptions<ResponseType>, | ||
): Promise<ResponseType | undefined> { | ||
emitter.emit('request:start', request) |
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.
Request cookies must not be set here, as they are already set in parseIsomorpchicRequest
.
If cookies are set here once more, they will get overridden in Node.js, as request cookies in Node.js are resolved to {}
(no location to check if the cookies should be inherited).
// Merge both direct request cookies and the cookies inherited | ||
// from other same-origin requests in the cookie store. | ||
mockedRequest.cookies = { | ||
...mockedRequest.cookies, |
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.
Surprisingly, there was no test that checks if the request cookies are properly set given the combination of its own cookies and those inherited from the cookie store. Previously, such combination resulted in the own request cookies being overridden, with the store cookies taking priority.
Here, I swap the cookies priority, with the request's own cookies potentially overriding the store (inherited) cookies, as that seems like something you'd expect.
}) | ||
}, | ||
onMockedResponse(response) { | ||
channel.send({ |
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.
It's important to signal the worker ignoring the response's delay, as the worker resolves the response respecting that delay. Otherwise, the delay would get doubled.
@@ -1,37 +1,51 @@ | |||
import * as https from 'https' | |||
import { rest } from 'msw' | |||
import { setupServer } from 'msw/node' | |||
import { setupServer, SetupServerApi } from 'msw/node' | |||
import { ServerApi, createServer } from '@open-draft/test-server' |
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'm using this pull request as an opportunity to remove external HTTP servers dependency from tests and replace it with the locally running server that represents a production one (which mustn't be hit in most test cases).
ded0247
to
3634836
Compare
cc2107b
to
3cbbb57
Compare
f333479
to
f97dc25
Compare
Bundling
|
Changes
handleRequest
function that encapsulates:getResponse
.HeadersList
. Instead, serializes mock response headers into aFlatHeadersObject
so that their shape is the same across regularstart
(where serialization is necessary to transmit the headers to the worker) and the fallbackstart
(where serialization is only necessary to respond properly).Motivation
file://
is forbidden.GitHub
Roadmap
interceptor
when callingworker.start()
andworker.stop()
without disrupting the public API while keeping the internals clean.debug
transient dependency ofinterceptors
property (currently pulls inutil
andtty
dependencies being bundled for UMD and ESM targets).