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

[feature] add h2 / spdy support #1302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[feature] add h2 / spdy support #1302

wants to merge 1 commit into from

Conversation

gabrielcsapo
Copy link

@gabrielcsapo gabrielcsapo commented Oct 2, 2018

What did I do?

Set up the initial pieces for serving http2 content in testem

screen shot 2018-10-01 at 10 18 37 pm

What needs to be done

  • Leverage server push to push down assets on initial load (using this.config.getServeFiles we can get all the files needed and res.push them on initial load)

@stefanpenner
Copy link
Contributor

stefanpenner commented Oct 2, 2018

I think moving to h2 makes sense, but @gabrielcsapo some I questions (mostly to understand impact):

  • how does the h2 perform relative to http1.1 given an app with many assets?
  • given ^, how much does push improve ^.

@step2yeung
Copy link
Member

step2yeung commented Oct 2, 2018

+1 on Stefan's comment on performance impact.

One caveat that I think needs discussion is that the cert used by testem will be self signed certs that the browsers will likely first redirect to a warning page, which I feel could be a breaking change. (Unless we there are flags for browsers that can bypass it?)

@stefanpenner
Copy link
Contributor

@step2yeung good point, could you investigate further?

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Why do we have both a package-lock.json and yarn.lock?

@@ -45,6 +46,7 @@
"rimraf": "^2.4.4",
"socket.io": "^2.1.0",
"spawn-args": "^0.2.0",
"spdy": "^3.4.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly concerned that this is unmaintained...

Is there any reason we can't use the built-in http2 module when present?

Copy link
Author

Choose a reason for hiding this comment

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

Right yeah I am under the mind that we should be using the built in modules for this instead of using express, which has had an open RB for almost 3 years for this support.

@step2yeung
Copy link
Member

Chrome has --allow-insecure-localhost switch to allow this, while Firefox has
network.websocket.allowInsecureFromHTTPS that can be added to the profile

But i don't think there are similar flags that is accepted by other supported browsers.

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.

None yet

4 participants