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

[now-cli][now-client] Add optional HTTP/2 #2989

Closed
wants to merge 5 commits into from

Conversation

@rdev
Copy link
Member

commented Sep 10, 2019

This PR implements two things:

  • HTTP/2 optionality in now-client (now HTTP/2 is off by default)
  • Makes now-cli use HTTP/2 on ^10 Node versions that have stable support

HTTP/2 now-client API

import { createDeployment,createLegacyDeployment } from 'now-client'

const useHttp2 = true

for await(event for createDeployment(path, opts, useHttp2)) {
  // ...
}

for await(event for createLegacyDeployment(path, opts, useHttp2)) {
  // ...
}
rdev added 2 commits Sep 10, 2019

@rdev rdev requested review from TooTallNate and styfle Sep 10, 2019

@rdev rdev requested a review from leo as a code owner Sep 10, 2019

@rdev rdev requested a review from AndyBitz Sep 10, 2019

@styfle styfle changed the title Add optional HTTP/2 to `now-client` and `now-cli` [now-cli][now-client] Add optional HTTP/2 Sep 10, 2019

@@ -18,12 +20,17 @@ export default async function processDeployment({
}: any) {
const { warn, log } = createOutput({ debug });
let bar: Progress | null = null;
const useHttp2 = semver.satisfies(pkg.version, '>10');

This comment has been minimized.

Copy link
@styfle

styfle Sep 10, 2019

Member

I don't think this is correct. You need to look at the current version of node running. Probably use process.versions.node.

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Sep 10, 2019

Member

process.version

This comment has been minimized.

Copy link
@rdev

rdev Sep 11, 2019

Author Member

Oh crap. Right

@TooTallNate

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Why isn't useHttp2 part of the opts?

rdev and others added 2 commits Sep 11, 2019
Update packages/now-cli/src/util/deploy/process-deployment.ts
Co-Authored-By: Nathan Rajlich <n@n8.io>
@@ -18,12 +19,17 @@ export default async function processDeployment({
}: any) {
const { warn, log } = createOutput({ debug });
let bar: Progress | null = null;
const useHttp2 = semver.satisfies(process.version, '>10');

This comment has been minimized.

Copy link
@styfle

styfle Sep 11, 2019

Member
Suggested change
const useHttp2 = semver.satisfies(process.version, '>10');
const useHttp2 = semver.satisfies(process.version, '>=10.4.0');

The docs mention that fetch-h2 requires Node 10.4.0 or newer https://www.npmjs.com/package/fetch-h2#releases

@styfle

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@rdev Is this PR still necessary or does #3011 supersede it?

@rdev

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

Closing this as we have decided not to implement HTTP/2 for now

@rdev rdev closed this Sep 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.