Skip to content

http: add http.createStaticServer - #45096

Open
aduh95 wants to merge 34 commits into
nodejs:mainfrom
aduh95:http-server
Open

aduh95 wants to merge 34 commits into
nodejs:mainfrom
aduh95:http-server

Conversation

@aduh95

@aduh95 aduh95 commented Oct 20, 2022 •

Copy link
Copy Markdown
Contributor

This PR adds a small utility to spawns a really simple HTTP 1.1 server to serve a local directory. The server is not meant to be secure or extremely performant, it's meant to be minimal and a useful replacement for python -m http.server. This should never be used in production of course.

Usage:

node -e 'http.createStaticServer()' # starts serving the cwd on a random port

# To start serving on the port 8080 using /path/to/dir as the root:
node -e 'http.createStaticServer({directory: "/path/to/dir", port: 8080})'

# Same as above, but exposing your local file system to the whole IPv4 network:
node -e 'http.createStaticServer({directory: "/path/to/dir", port: 8080, host: "0.0.0.0"})'

Semver-major if we want to release it without requiring the node: prefix.
EDIT: I've removed the node:http/static module from this PR, it can be discussed in a separate PR.

Fixes: #45079

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 20, 2022
@nodejs-github-bot nodejs-github-bot added http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Oct 20, 2022
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
Comment thread doc/api/http.md Outdated
node -r node:http/server # starts serving the cwd on a random port

# To start serving on the port 8080 using /path/to/dir as the root:
node -r node:http/server /path/to/dir --port 8080

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.

Not really a fan of building in a preload like this. Someone can easily use the -pe syntax to achieve this if they wanted, e.g. node -pe "require('http').createStaticServer(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One reason to use -r is you can get autocomplete when selecting the root directory, and it's easier to offer shorter versions of flags (i.e. I'd rather type node -r http/server . -p 8080 -h 0.0.0.0 than node -e 'http.createStaticServer({port: 8080, host: '0.0.0.0'})'). Currently this PR offers both syntax, so I guess I should also document that, and folks can chose the syntax they prefer, wdyt?

Comment thread lib/http/server.js Outdated
Comment thread lib/http/server.js Outdated

emitExperimentalWarning('http/server');

if (module.isPreloading) {

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.

This is dangerous. If someone has their own preload module and happens to require('http/server') within it, this part will be run while they may not expect it.

Comment thread lib/http/server.js Outdated
Comment thread lib/http/server.js Outdated
Comment thread lib/http/server.js Outdated
Comment thread lib/http/server.js Outdated

@mcollina mcollina left a comment

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.

This needs tests.

I would also recommend we implement:

  • partial responses (Ranges)
  • conditional-GET negotiation (If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since) - this includes etag generation.

It's not necessary that those are implemented now, however we should note those are not implemented in the docs.

Comment thread lib/http/server.js
stream.on('error', reject);
stream.on('close', resolve);
stream.pipe(res);
});

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.

You should use stream.pipeline instead if this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pipeline implementation seems overly complicated in comparison of what's needed here (we don't need any AbortSignal support, etc.), and tries to do more that we want (i.e. it closes the request in case of error before we can send a 404 status).

Comment thread doc/api/http.md Outdated
@aduh95
aduh95 marked this pull request as ready for review October 23, 2022 02:49
@aduh95

aduh95 commented Oct 23, 2022

Copy link
Copy Markdown
Contributor Author

Tests have been added, this is ready for reviews. There's a discussion regarding the name of the module (http/server, or http/static, or?) in #45096 (comment), if you have thoughts please chime in.

@aduh95

aduh95 commented Oct 23, 2022

Copy link
Copy Markdown
Contributor Author

There are a few CI failures that need to be taken care of in separate PRs, converting this one back to draft.

@aduh95
aduh95 marked this pull request as draft October 23, 2022 16:00
@aduh95 aduh95 changed the title http: add node:http/server http: add node:http/static Oct 26, 2022
@aduh95
aduh95 marked this pull request as ready for review October 26, 2022 01:11
@aduh95
aduh95 requested review from jasnell and mcollina October 26, 2022 01:17
Comment thread lib/http/static.js Outdated
const { validateFunction, validatePort } = require('internal/validators');

if (module.isPreloading) {
const requiredModules = getOptionValue('--require');

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.

Still very much not a fan of building preload into this.

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.

Keep in mind, for instance, that -r is permitted in the NODE_OPTIONS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you have in mind what changes we would want to bring before we're happy with this pattern? Or do you think the pattern itself is undesirable? Personally I quite like the UX of it, and I think it's better than coming up with another built-in flag, but I'm happy to withdraw it if I'm in the minority.

Comment thread lib/http/static.js
@aduh95

aduh95 commented Nov 23, 2022

Copy link
Copy Markdown
Contributor Author

@mcollina are you still blocking this?

@mcollina mcollina left a comment

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.

lgtm

@vlourenco-stf vlourenco-stf left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Non-blocking comments, just things to make it a little bit faster.

Comment thread lib/internal/http/static.js Outdated
Comment thread lib/internal/http/static.js

try {
try {
const stream = createReadStream(url, { emitClose: false });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I know is a nitpick but I think you can cache this object to avoid creating every time.

Also, maybe expose the highWaterMark to the user configure is a good idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean, do you mean cache the stream? We're going to need a different stream for each request, I think, so I don't see how it'd be useful 🤔 could you send an example maybe?

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.

Caching { emitClose: false }

@ruyadorno ruyadorno left a comment

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.

LGTM 👍

@ruyadorno ruyadorno added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 4, 2024
@github-actions

github-actions Bot commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @ruyadorno.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 4, 2024
@tniessen

tniessen commented Apr 4, 2024

Copy link
Copy Markdown
Member

Should there be a note about security in the docs? Specifically, I am wondering what would constitute a vulnerability here.

@ruyadorno

Copy link
Copy Markdown
Member

@aduh95 heads up, please update the usage example in the body so that releasers & other collaborators can potentially refer to it.

@TheOneTheOnlyJJ

Copy link
Copy Markdown
Contributor

Any update on this?

@metcoder95

Copy link
Copy Markdown
Member

Is there anything missed to move the PR forward? Happy to support it if needed

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@loynoir

loynoir commented Jul 28, 2026

Copy link
Copy Markdown

Would be nice to resolve conflict and merge.

@github-actions github-actions Bot removed the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@aduh95

aduh95 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor Author

Given nodejs/security-wg#1274 (comment), I'm unsure this can go forward without first defining a new status for APIs that we want to guarantee semverness but not security

@panva panva removed the review wanted PRs that need review. label Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues and PRs related to the http subsystem. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Implement a way to expose static files from a directory (similar to Python SimpleHttpServer)