Skip to content

Add quic support#15

Open
agilan2001 wants to merge 3 commits into
moscajs:mainfrom
agilan2001:add-quic-support
Open

Add quic support#15
agilan2001 wants to merge 3 commits into
moscajs:mainfrom
agilan2001:add-quic-support

Conversation

@agilan2001
Copy link
Copy Markdown

Added support for QUIC protocol through the core QUIC module of NodeJS (v15 - pre)

NodeJS QUIC Documentation

@robertsLando robertsLando requested a review from getlarge July 1, 2021 12:10
Copy link
Copy Markdown
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Fix lint issues. Also could you submit a test for that?

@agilan2001 agilan2001 closed this Jul 1, 2021
@agilan2001 agilan2001 reopened this Jul 1, 2021
@agilan2001
Copy link
Copy Markdown
Author

Added support for QUIC protocol through the core QUIC module of NodeJS (v15 - pre)

NodeJS QUIC Documentation

Builds pass only with the NodeJS 15 pre version (manual build) currently.

Copy link
Copy Markdown
Member

@getlarge getlarge left a comment

Choose a reason for hiding this comment

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

@agilan2001 Like @robertsLando said, we should have some tests to check that it works.
The problem is that QUIC seems to be only supported on Node 15 with a custom build, right ?
So i wonder how that would fit in the current tests and the CI workflow, maybe with an environment variable ?

Comment thread index.js
Comment on lines +73 to +80
server = createQuicSocket({
endpoint: { port: options.quic.port || 8885 }, // default port 8885 if not mentioned in options
server: {
key: options.quic.key,
cert: options.quic.cert,
alpn: options.quic.alpn || 'mqtt'
}
})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When i look at the Node15 API doc, there are a lot more options available, why did you ignore them ?

Comment thread types/index.d.ts
http2?: Http2ServerOptions;
tls?: TlsOptions;
tcp?: { allowHalfOpen?: boolean; pauseOnConnect?: boolean };
quic?:{ port?: number; key: ArrayBuffer; cert: ArrayBuffer; alpn: string};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are you sure, only ArrayBuffer is allowed for key and cert ?

@robertsLando
Copy link
Copy Markdown
Member

About tests I would check node major using process.version and run tests only on nodejs > 15

Copy link
Copy Markdown

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm -1 to add this. The QUIC implementation is coming back to Node.js in the next month or so, let's wait for it.

@robertsLando
Copy link
Copy Markdown
Member

@mcollina Is there any problem if we allow it only on allowed nodejs versions?

@mcollina
Copy link
Copy Markdown

mcollina commented Jul 2, 2021

There are no nodejs binaries with quic enabled.

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