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

Skips "opaque" responses when handling life-cycle events #564

Merged
merged 6 commits into from
Jan 24, 2021

Conversation

kettanaito
Copy link
Member

GitHub

Changes

  • Response event handler now skips "opaque" responses (those originating from requests with mode: "no-cors") based on their response.type value.
  • Improves type annotation of the response object sent from the worker (it's only a partial representation of the Response type).

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 24, 2021

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 5a54e08:

Sandbox Source
MSW React Configuration

@kettanaito
Copy link
Member Author

Hey, @timdeschryver. Thank you for writing this test. If you don't mind, I've polished it but a little. I would appreciate your review if you have a spare minute. Thanks!

@kettanaito
Copy link
Member Author

I've replaced the image example with a local test server with a strict CORS policy. This produces the same exception without the fix, making the test fail. The main reason for doing so is to remove any external dependency (such as on an external image provider) in our tests. It should be possible to run all test suites offline, which implies no dependencies on actual servers. We can substitute actual servers with the @open-draft/test-server package.

@kettanaito
Copy link
Member Author

I believe that the Codesandbox build won't succeed with this breaking change to the Service Worker file. Without the addition of the type property in the response sent to the client, the following like will throw indefinitely:

if (responseJson.type.includes('opaque')) {

While this isn't nice, we are going to publish this in the next minor release, indicating a breaking change. That change will imply that you have to update your worker script by npx msw init.

@kettanaito kettanaito added BREAKING CHANGE Pull request introducing breaking changes. release candidate labels Jan 24, 2021
Copy link
Member

@marcosvega91 marcosvega91 left a comment

Choose a reason for hiding this comment

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

Contrary to what I have said on discord if there is a network error the service worker will not call the function sendToClient. I suggest only adding an extra as in the comment above, then for me is super ok :)

src/utils/worker/createResponseListener.ts Outdated Show resolved Hide resolved
@kettanaito
Copy link
Member Author

Actually, I think you're right to introduce the safety check, @marcosvega91. I've tested this once more and it produces an infinite loop with an outdated worker. I don't think that's a good experience to aim for.

I will add a safety check that will be removed in the minor release after the next one. We'll give it 1 version to bake in.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the cleanup!

Copy link
Member

@marcosvega91 marcosvega91 left a comment

Choose a reason for hiding this comment

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

Nice work 💪🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE Pull request introducing breaking changes. release candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to construct 'Response': The status provided (0) is outside the range [200, 599]
3 participants