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

SSE Bypass for HMR #834

Closed
takefumi-yoshii opened this issue Jul 27, 2021 · 10 comments · Fixed by #837
Closed

SSE Bypass for HMR #834

takefumi-yoshii opened this issue Jul 27, 2021 · 10 comments · Fixed by #837
Assignees
Labels
bug Something isn't working needs:tests

Comments

@takefumi-yoshii
Copy link
Contributor

Describe the bug

When developing on the Next.js with next dev, No problem for a while,
but suddenly the server becomes unresponsive.

Environment

  • msw: 0.29.0
  • nodejs: 14.16.0
  • npm: 0.0.0
  • next: 10.2.3

Please also provide your browser version.

Chrome: Version 91.0.4472.164 (Official Build) (x86_64)

To Reproduce

Steps to reproduce the behavior:

  1. npm run next dev
  2. Edit some code many times.
  3. Suddenly the server becomes unresponsive.

Expected behavior

Server always responds.

Screenshots

The below capture is the Network tab when no response is returned.

image

Workaround

It looks like there is a problem in webpack-hmr?page=xxxx.
As a workaround, I added the below SSE Bypass in generated mockServiceWorker.js.
Then, I was able to solve the problem that the response did not come back.

self.addEventListener("fetch", function (event) {
  const { request } = event;

  // Bypass server-sent events.
  if (request.headers.get("accept") === "text/event-stream") {
    return;
  }

  // Bypass navigation requests.
  if (request.mode === "navigate") {
    return;
  }
  ...
}

This Bypass is useful to us, but what do you think?

@takefumi-yoshii takefumi-yoshii added the bug Something isn't working label Jul 27, 2021
@kettanaito
Copy link
Member

Hey, @takefumi-yoshii. Thank you for reporting and investigating this!

Do you think this may be the root cause for #785 and #788?

I was mistaken to believe that Service Worker cannot intercept SSE, but that's not the case. Looks like we'd have to bypass such requests for now, at least until #156 lands.

Please, would you be interested in opening a pull request with your fix?

@takefumi-yoshii
Copy link
Contributor Author

Do you think this may be the root cause for #785 and #788?

I feel that both are related, but I can't reply without reproduce environment.
But I think it's worth having a reporter try this bypass.
The same is true for developer who do not need to intercept SSE.

Please, would you be interested in opening a pull request with your fix?

Of course! I hope it helps until it is fundamentally resolved.
Wait a minute. @kettanaito

@kettanaito
Copy link
Member

kettanaito commented Jul 28, 2021

I've tried these steps to reproduce using [email protected] and our NextJS usage example. I've updated the code ~100 times and all HMR-related requests are still processed properly. The difference I see is that there's no /_next/webpack-hmr request at all, I only get JavaScript chunks in the Network.

Can I try something else to reproduce this issue?

Edit: what triggers the /webpack-hmr request for me is when I switch from and back to the tab of my application in the browser. Doing so repeatedly (~15 times) causes that request to fail without any response. I believe that's what's happening.

@kettanaito
Copy link
Member

So the issue is, in fact, related to how MSW processed repeatedly established SSE from webpack. Looks like webpack HMR has some logic that triggers SSE whenever you come back to the application's tab in the browser. It'd be nice to find that logic in their source code and see how we can emulate it in a test suite.

We do run tests in an automated Chromium, so we can switch from/to the tab without issues. That won't trigger SSE by default, though. I suspect there's some logic on webpack's side that does that.

@takefumi-yoshii
Copy link
Contributor Author

takefumi-yoshii commented Jul 28, 2021

I also reproduced it with NextJS usage example .It happened when repeated tab switching.I also look for the relevant part of webpack.

@kettanaito
Copy link
Member

The most reliable way to test this will be to apply webpack-hot-middleware on the testing server directly and emulate tabs switching in Chromium. I'm researching how to do that with page-with...

@takefumi-yoshii
Copy link
Contributor Author

In devtool waterfall, eventsource is disconnected by tab switching, but fetch is not.
If could disconnect the fetch, could the problem be solved ...?

image

@kettanaito
Copy link
Member

I'd rather find out more about why eventsource gets disconnected. I wonder if there's any reason we can introspect?
But in a nutshell, such cancellation would have to be explicit, and I'm somewhat not convinced this is something MSW should do. That eventsource cancellation happens for some preason, which is either how Service Worker handlers repetitive SSE, or something done by webpack. Neither should be handled by the library.

I'm quite content with the fix you've provided, to bypass any text/event-stream requests for now. My main concern here is writing a reliable test for this.

@kettanaito kettanaito self-assigned this Jul 28, 2021
@kettanaito
Copy link
Member

Okay, I've set up a basic WebpackDevServer with HMR and can see it is enabled in the browser:

[HMR] Waiting for update signal from WDS...
index.js:49 [WDS] Hot Module Replacement enabled.
index.js:53 [WDS] Live Reloading enabled.

However, no /webpack-hmr requests are being made: neither initially, nor when switching from/to the tab. Yet again something else is at play apart from webpack+HMR.

@kettanaito
Copy link
Member

Analyzing NextJS

Here's where the EventSource is being established on the client:

evtSource = getEventSourceWrapper({
  path: `${assetPrefix}/_next/webpack-hmr?page=${currentPage}`,
  timeout: 5000,
})

next.js/packages/next/client/dev/on-demand-entries-utils.js

The server-side of NextJS just bypasses this route:

It appears that the SSE established is not coming from webpack but from NextJS. Huh, so there's no straightforward way to put this in an integration test in this repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working needs:tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants