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

feat: automatically enable allowHTTP1 by default with http2 & hmr #186

Closed

Conversation

illusionalsagacity
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Changes the default configuration for the http2 server if hmr is true.

Please Describe Your Changes

relates to #183 and #184

I added the tls-keygen dev dependency in order to generate the keys, unfortunately browsers do not seem to support unencrypted http2 at this time, even on localhost. Probably worth some followup work to make the http2 option require some of the http2.createSecureServer options to avoid any footguns?

Copy link
Owner

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

I see the new test fixtures added, but not any tests. Is that pending or uncommitted?

README.md Outdated
@@ -138,7 +138,7 @@ If `true`, will enable [`Hot Module Replacement`](https://webpack.js.org/concept

_Note: If the build process generates errors, the client (browser) will not be notified of new changes and no HMR will be performed. Errors must be resolved before HMR can proceed._

_Note: If using in combination with `http2`, the `http2` option `allowHTTP1` must be enabled for the HMR WS connection to work._
_Note: If using in combination with `http2`, the `http2` option `allowHTTP1` will enabled by default for the HMR WS connection to work._
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
_Note: If using in combination with `http2`, the `http2` option `allowHTTP1` will enabled by default for the HMR WS connection to work._
_Note: If using in combination with `http2`, the `http2` option `allowHTTP1` will be enabled by default for the HMR WS connection to work._

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, pending I will add another case for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I can't get the integration tests to run to completion locally, particularly the multi-hmr one seems to be troublesome. It does happen as well on the master branch, so I'm not quite sure what is going on there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you tell more about the test issue? How does it fail exactly?

@shellscape
Copy link
Owner

@illusionalsagacity I'll try and step into your branch to get the tests work. probably later this week.

@illusionalsagacity
Copy link
Contributor Author

@illusionalsagacity I'll try and step into your branch to get the tests work. probably later this week.

Alright, thanks. To answer @bebraw the issue I'm seeing is that chromium just sits on the about:blank page and the tests hang for me. Probably related to some of the changes I made with the puppeteer config, I would assume.

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

Successfully merging this pull request may close these issues.

4 participants