add node:http and node:https documentation #24268
Conversation
anonrig
requested review from
GregBrimble,
irvinebroque,
mikenomitch,
korinne,
WalshyDev,
kodster28 and
a team
as code owners
github-actions
bot
assigned
GregBrimble,
irvinebroque,
kodster28,
korinne,
mikenomitch and
WalshyDev
|
This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:
|
|
We should probably mark this draft or hold off on merging until the server side stuff is actually rolled out without the experimental flag. |
|
|
||
| The `request` method creates an HTTP request with customizable options like method, headers, and body. It provides full control over the request configuration and returns a [writable stream](https://nodejs.org/docs/latest/api/stream.html#class-streamwritable) for sending request data. | ||
|
|
||
| ```js | ||
| import { request } from 'node:http'; | ||
| import { strictEqual } from 'node:assert'; | ||
| import { strictEqual, ok } from 'node:assert'; |
There was a problem hiding this comment.
these examples are, unfortunately, not directly runnable in workers since they show the requests at the top level scope (where we can't perform an i/o). This can be addressed in a follow-up but ideally these examples would show the use of request and get as subrequests in a request to show how they would actually be used. Probably also a good idea to show the proper use of await or waitUntil to show how to properly wait on them.
There was a problem hiding this comment.
I agree that would be a good improvement, but it's not really related to this PR which just adds the missing modules and methods.
Mind if we track this separately? It's a bigger change that probably deserves its own discussion about how we want to handle the examples and async patterns.
Happy to work on it after this gets merged if you want.
There was a problem hiding this comment.
My preference would be to improve the examples before it lands but I'll leave it to you.
anonrig
force-pushed
the
yagiz/add-nodejs-http-https-modules
branch
from
68d2271 to
6810838
Compare
| export default httpServerHandler({ port: 8080 }); | ||
| ``` | ||
|
|
||
| The `httpServerHandler` function integrates Node.js HTTP servers with the Cloudflare Workers request model. When a request arrives at your Worker, the handler automatically routes it to your Node.js server running on the specified port. This bridge allows you to use familiar Node.js server patterns while benefiting from the Workers runtime environment, including automatic scaling, edge deployment, and integration with other Cloudflare services. |
There was a problem hiding this comment.
I'm not sure about this way of explaining it. httpServerHandler returns a workers handler object that implements the fetch export, the fetch export uses the port as a key to determine which app to forward to. The way this is worded it gives the impression that the node:http server is actually running on an actual port. Just comes across a bit misleading.
There was a problem hiding this comment.
Good point. The current wording makes it sound like there's an actual server running on a real port, when it's really using the port as a routing key.
How would you suggest rewording this to be more accurate? Something along the lines of the handler using the port to determine which app to route to?
anonrig
force-pushed
the
yagiz/add-nodejs-http-https-modules
branch
2 times, most recently
from
bed92bb to
4dc141b
Compare
anonrig
force-pushed
the
yagiz/add-nodejs-http-https-modules
branch
from
4dc141b to
5f46450
Compare
| @@ -51,13 +75,13 @@ get('http://docs.cloudflare.com/robots.txt', (res) => { | |||
|
|
|||
| ## request | |||
|
|
|||
| An implementation of the Node.js [`http.request'](https://nodejs.org/docs/latest/api/http.html#httprequesturl-options-callback) method. | |||
| An implementation of the Node.js [`http.request`](https://nodejs.org/docs/latest/api/http.html#httprequesturl-options-callback) method. | |||
|
|
|||
| The `request` method creates an HTTP request with customizable options like method, headers, and body. It provides full control over the request configuration and returns a [writable stream](https://nodejs.org/docs/latest/api/stream.html#class-streamwritable) for sending request data. | |||
|
|
|||
| ```js | |||
| import { request } from 'node:http'; | |||
There was a problem hiding this comment.
Better to replace this example with a more fully functional one that shows the request used in the correct way (i.e. as a subrequest within a request handler where there is an i/o context, with proper error propagation, a promise that gets resolved for the response, etc).
import { request } from 'node:http';
export default {
fetch() {
const { promise, resolve, reject } = Promise.withResolvers();
request({
method: 'GET',
protocol: 'http:',
hostname: 'example.com',
path: '/'
}, (res) => {
let data = '';
res.setEncoding('utf8');
res.on('data', (chunk) => {
data += chunk;
});
res.once('end', () => {
resolve(new Response(data));
});
res.once('error', reject);
}).once('error', reject)
.end();
return promise;
}
}
| @@ -91,7 +118,7 @@ const req = request(new URL('http://docs.cloudflare.com'),{ | |||
| req.end(); | |||
| ``` | |||
There was a problem hiding this comment.
I would just remove this second example as it does not really add much value. You can just use text to explain that the input can be an object, URL string, or URL object.
| @@ -110,9 +139,12 @@ res.writeHead(200, { 'Content-Type': 'text/plain' }); | |||
| res.write('Hello, World!'); | |||
| res.end(); | |||
| ``` | |||
There was a problem hiding this comment.
Given that it would be exceedingly unlikely that the typical use would ever need to create an OutgoingMessage themselves, this example is not all that useful. Instead, I would point out that the ClientRequest and ServerResponse both extend from and inherit from OutgoingMessage and remove the example
| @@ -123,3 +155,123 @@ get('http://docs.cloudflare.com', (res) => { | |||
| ok(res instanceof IncomingMessage); | |||
| }); | |||
There was a problem hiding this comment.
import { get, IncomingMessage } from 'node:http';
import { ok, strictEqual } from 'node:assert';
export default {
fetch() {
const { promise, resolve } = Promise.withResolvers();
get('http://docs.cloudflare.com', (res) => {
strictEqual(res.statusCode, 301);
ok(res instanceof IncomingMessage);
resolve(new Response('ok');
});
return promise;
}
}
| // Access Cloudflare-specific request properties | ||
| console.log(res.cloudflare.cf.country); | ||
| console.log(res.cloudflare.cf.ray); | ||
| }); |
There was a problem hiding this comment.
the cloudflare.cf is a server-side property on Request objects. It would not be included on fetch response, so this example is not correct.
| await using server = createServer((req, res) => { | ||
| res.end('Hello World'); | ||
| }); | ||
| // Server will be automatically closed when it goes out of scope |
There was a problem hiding this comment.
In this example, the server would never actually be retained because it's not listening. Also since the await using is at the top level scope, even if it was listening, it would end up being immediately closed when the top-level evaluation scope exits before any requests could be received and processed, so this example is buggy and incorrect as is.
|
|
||
| The `Server` class represents an HTTP server and provides methods for handling incoming requests. It extends the Node.js `EventEmitter` class and can be used to create custom server implementations. | ||
|
|
||
| When using `httpServerHandler`, the port number specified in `server.listen()` acts as a routing key rather than an actual network port. The handler uses this port to determine which HTTP server instance should handle incoming requests, allowing multiple servers to coexist within the same Worker by using different port numbers for identification. |
There was a problem hiding this comment.
It's also worth pointing out that passing a port value of 0 (or null or undefined) will result in a random port number being assigned.
| The following differences exist between the Workers implementation and Node.js: | ||
|
|
||
| - Trailer headers are not supported | ||
| - The `socket` attribute **does not extend from `net.Socket`** and only contains the following properties: `encrypted`, `remoteFamily`, `remoteAddress`, `remotePort`, `localAddress`, `localPort`, and `destroy()` method |
There was a problem hiding this comment.
Should likely explain that calling msg.socket.destroy() just falls through to msg.destroy(). Otherwise, it would probably be good to document these a bit more (e.g. explaining the expected remotePort range, what remoteAddress represents, the fact that localAddress is generally non-op, etc and possibly provide links to the node.js equivalent documentation for these. Can be done in a separate PR tho.
| The following differences exist between the Workers implementation and Node.js: | ||
|
|
||
| - Connection management methods such as `closeAllConnections()` and `closeIdleConnections()` are not implemented | ||
| - Only `listen()` variants with a port number or no parameters are supported: `listen()`, `listen(0, callback)`, `listen(callback)`, etc. |
There was a problem hiding this comment.
General users may not be too familiar with the fact that Node.js' API allows listening on a domain socket, etc so this may be a bit confusing. A link here to the relevant node.js docs would help.
| The `httpServerHandler` function integrates Node.js HTTP servers with the Cloudflare Workers request model. When a request arrives at your Worker, the handler automatically routes it to your Node.js server running on the specified port. This bridge allows you to use familiar Node.js server patterns while benefiting from the Workers runtime environment, including automatic scaling, edge deployment, and integration with other Cloudflare services. | ||
|
|
||
| :::note | ||
| Failing to call `close()` on an HTTP server may result in the server persisting until the worker is destroyed. In most cases, this is not an issue since servers typically live for the lifetime of the worker. However, if you need to create multiple servers during a worker's lifetime or want explicit lifecycle control (such as in test scenarios), call `close()` when you're done with the server, or use explicit resource management: |
There was a problem hiding this comment.
Users likely aren't going to be familiar with explicit resource management since it is so new. A link here to something that explains it would be most helpful.
|
|
||
| An implementation of the Node.js [`http.createServer`](https://nodejs.org/docs/latest/api/http.html#httpcreateserveroptions-requestlistener) method. | ||
|
|
||
| The `createServer` method creates an HTTP server instance that can handle incoming requests. It's a convenience function that creates a new `Server` instance and optionally sets up a request listener callback. |
There was a problem hiding this comment.
| The `createServer` method creates an HTTP server instance that can handle incoming requests. It's a convenience function that creates a new `Server` instance and optionally sets up a request listener callback. | |
| The `createServer` method creates an HTTP server instance that can handle incoming requests. |
It's not really a "convenience" method. It's the way users are supposed to create servers. The fact that new Server(...) is possible is a historical artifact of the original node.js implementation of the API. I would just drop the second sentence here.
|
|
||
| await using server = createServer((req, res) => { | ||
| res.end('Hello World'); | ||
| }); |
There was a problem hiding this comment.
As in the http.mdx case, this example is incorrect. The server is not listening (so it would never be retained) and the use of await using at the top level scope would be even if it were listening, it would be closed before any requests could be processed.
| - Connection management methods such as `closeAllConnections()` and `closeIdleConnections()` are not implemented | ||
| - Only `listen()` variants with a port number or no parameters are supported: `listen()`, `listen(0, callback)`, `listen(callback)`, etc. | ||
| - The following server options are not supported: `maxHeaderSize`, `insecureHTTPParser`, `keepAliveTimeout`, `connectionsCheckingInterval` | ||
| - TLS/SSL-specific options such as `ca`, `cert`, `key`, `pfx`, `rejectUnauthorized`, `secureProtocol` are not supported in the Workers environment |
There was a problem hiding this comment.
It would be most helpful if these all included links to the relevant parts of the node.js docs. Can be added in a separate PR tho.
| | HTTP/2 | ⚪ not yet supported | | ||
| | HTTPS | 🟡 partially supported | | ||
| | HTTPS | 🟢 supported | |
There was a problem hiding this comment.
I actually think it might be good to keep this as "partially supported" since there's a fair amount of the https options that aren't implemented or are non-ops that we currently have no plans to support. (If the gaps are just temporary, then that's different)
There was a problem hiding this comment.
I would say exactly the opposite.
If we believe this is what full support looks like, we should say supported.
If anyone is missing something they can create a ticket.
|
|
||
| An implementation of the Node.js [`https.Server`](https://nodejs.org/docs/latest/api/https.html#class-httpsserver) class. | ||
|
|
||
| The `Server` class represents an HTTPS server and provides methods for handling incoming secure requests. It extends the Node.js `EventEmitter` class and can be used to create custom secure server implementations. |
There was a problem hiding this comment.
Suggest something like this instead:
In Node.js, the `https.Server` class represents an HTTPS server and provides
methods for handling incoming secure requests. In Workers, handling of secure
requests is provided by the Cloudflare infrastructure so there really is not
much difference between using `https.Server` or `http.Server`. The workers
runtime provides an implementation for completeness but most workers should
probably just use `http.Server`.
likely with a link to the http.Server doc.
|
|
||
| Request method accepts all options from `http.request` with some differences in default values: | ||
| The request method accepts all options from `http.request` with some differences in default values: |
There was a problem hiding this comment.
Making http.request a link over to the http.mdx file would be helpful.
| }); | ||
|
|
||
| server.listen(8080); | ||
| export default httpServerHandler({ port: 8080 }); |
There was a problem hiding this comment.
The signature and behavior of the httpServerHandler(...) API needs to be documented in here as well.
There was a problem hiding this comment.
Note that a couple of the details of how httpServerHandler(...) works have been updated here: cloudflare/workerd#4735
Summary
Adds the documentation for node http and https modules.
Documentation checklist