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

HMR broken with historyFallback in Firefox #94

Closed
davidroeca opened this issue Jan 18, 2019 · 11 comments · Fixed by #98
Closed

HMR broken with historyFallback in Firefox #94

davidroeca opened this issue Jan 18, 2019 · 11 comments · Fixed by #98

Comments

@davidroeca
Copy link
Contributor

  • Webpack Version: 4.23.1
  • Operating System (or Browser): Linux Mint 18.2 Cinammon 64-bit (Firefox 65.0 and 64.0.2)
  • Node Version: 11.3.0
  • webpack-plugin-serve Version: 0.4.0

How Do We Reproduce?

With the given Firefox versions, run the react recipe in this repo.

Expected Behavior

HMR works as expected with historyFallback on Firefox. This is especially important if I'm developing an app that relies on something like https://github.com/ReactTraining/react-router for SPA purposes.

Actual Behavior

Notice in the console that the following error occurs: Firefox can’t establish a connection to the server at ws://[::]:55555/wps..

Now kill the start script, remove the historyFallback option and restart. The bug now should be fixed.

Note that this problem does not exist on current versions of Chrome/Chromium.

@davidroeca
Copy link
Contributor Author

davidroeca commented Jan 18, 2019

By changing configuration to the following:

    new Serve({
      // note: this value is true by default
      hmr: true,
      historyFallback: {
        logger: console.log.bind(console),
        verbose: true,
      },
      static: [outputPath]
    })

I found that when loading localhost:55555, the following is logged in a Firefox page load:

Rewriting GET / to /index.html
Not rewriting GET /bundle.js because the client does not accept HTML.
Rewriting GET /wps to /index.html

While the following behavior happens in Chrome:

Rewriting GET / to /index.html
Not rewriting GET /bundle.js because the client does not accept HTML.
Not rewriting GET /wps because the client did not send an HTTP accept header.
Not rewriting GET /favicon.ico because the client does not accept HTML.

@davidroeca
Copy link
Contributor Author

davidroeca commented Jan 18, 2019

Not sure if this is the most optimal, but it can be fixed with the following configuration:

    new Serve({
      // note: this value is true by default
      hmr: true,
      historyFallback: {
        logger: console.log.bind(console),
        verbose: true,
      },
      middleware: (app, builtins) => app.use(async (ctx, next) => {
        if (ctx.path.match(/^\/wps/)) {
          const { accept, ...remainingHeaders } = ctx.request.header
          ctx.request.header = remainingHeaders
        }
        await next()
      }),
      static: [outputPath]
    })

EDIT: a simpler approach

    new Serve({
      // note: this value is true by default
      hmr: true,
      historyFallback: {
        // disable the plugin on /wps
        rewrites: [
          {
            from: '/wps',
            to: (context) => (context.parsedUrl.pathname),
          },
        ],
      },
      static: [outputPath]
    })

@shellscape
Copy link
Owner

@davidroeca would you classify this as a problem with WPS or with http-proxy-middleware directly?

@davidroeca
Copy link
Contributor Author

@shellscape well actually, it's most likely an issue with how connect-history-api-fallback deals with WebSocket requests from Firefox vs Chrome, because the logic relies on the non-existence of an Accept header, which Firefox provides but Chrome does not. My most recent work-around essentially disables connect-history-api-fallback on the /wps path.

See https://github.com/bripkens/connect-history-api-fallback/blob/e931cce468099b8d08d3e6115e3e2b8b598e11d1/lib/index.js#L19

@shellscape
Copy link
Owner

Ah yeah, that's what I meant :) mental lapse there. Could this be related? bripkens/connect-history-api-fallback#60

@davidroeca
Copy link
Contributor Author

Somewhat related, though Firefox sends along an Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 header with websockets and I haven't seen chrome replicate this behavior with the same request.

So I guess for now it's a bug specific to websockets in firefox, which passes along an Accept header to connect-history-api-fallback, which then rewrites to /index.html.

@shellscape
Copy link
Owner

Thanks for the explanation. Do you have any suggestions for how we should best handle this?

@davidroeca
Copy link
Contributor Author

Maybe an option for the wps web sockets to be configured on a separate port, similar to webpack-serve? In that case, the websocket portion won't need to interact with the history fallback.

@shellscape
Copy link
Owner

That would require using a separate WebSocket Server, something that we explicitly tried to avoid because of the complications and overhead it created. Unfortunately that's not an option for this plugin. I can definitely add a FAQ question for this. But I'd be curious to know about any other ideas you have. If adding the middleware from your reply above fixes the issue, I'm open to making that a permanent addition as well.

@davidroeca
Copy link
Contributor Author

davidroeca commented Jan 25, 2019

Yeah I mean implementing a middleware that gets around this definitely makes sense to me as long as it doesn't make the plugin code too confusing. Maybe by default this middleware behavior is turned on but there could be a configuration option to turn it off if it causes people problems? It would probably be worth documenting this behavior as it relates to connect-history-api-fallback because removing the accept header might cause confusion for some people

@shellscape
Copy link
Owner

Sounds like a plan to me. I'll get this implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants