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.

Sign up for GitHub

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

Jump to bottom

Replace express with builtin node server #398

Merged
merged 5 commits into from Dec 1, 2020
Merged

Replace express with builtin node server #398

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a674c4
Replace express with builtin node server
TrySound Nov 28, 2020
776ecb7
Add missing require
TrySound Nov 30, 2020
b30784d
Remove viewsRoot
TrySound Nov 30, 2020
ab9f23d
Switch to sirv
TrySound Nov 30, 2020
0f70814
Drop static-serve, add comment
TrySound Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 48 additions & 19 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -37,11 +37,11 @@
"acorn-walk": "^8.0.0",
"chalk": "^4.1.0",
"commander": "^6.2.0",
"express": "^4.17.1",
"filesize": "^6.1.0",
"gzip-size": "^6.0.0",
"lodash": "^4.17.20",
"opener": "^1.5.2",
"serve-static": "^1.14.1",
Copy link
Member

Choose a reason for hiding this comment

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

Seems like as per this comment:

#212 (comment)

serve-static is a big reason why express is so large so if polka would work for our use case, it's much smaller than using serve-static.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that other PR uses sirv for the static serving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if you are ok to introduce sirv I will use it. I just remember there was a doubt to introduce polka packages.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I don't really know what opinion @th0r has 😅 — sorry for being unclear. If things work then the size differences this provides might be useful?

As long as the server is still able to show new results when a bundle is changed like was the purpose behind introducing this feature back in #74 then I would be OK with this change.

I think the supported node versions are good enough, too, and we don't accidentally regress from supporting node v10.13.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support watch I passed "dev" flag which prevents caching.
I guess the size will be almost the same but without more polka dependencies.
#212 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

The serve-static is still in dependencies so I don't know how to figure out the size difference yet — some numbers on how this potentially affects the size of node_modules would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://packagephobia.com/result?p=static-serve
https://packagephobia.com/result?p=express (depends on static-serve out of the box)
https://packagephobia.com/result?p=sirv
https://packagephobia.com/result?p=polka (does not include sirv out of the box)
https://packagephobia.com/result?p=%40polka%2Furl (dependency of both sirv and polka)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need serve-static if you decided to use sirv instead?

"ws": "^7.3.1"
},
"devDependencies": {
Expand Down
42 changes: 26 additions & 16 deletions src/viewer.js
View file
Open in desktop
Expand Up @@ -3,8 +3,8 @@ const fs = require('fs');
const http = require('http');

const WebSocket = require('ws');
const serveStatic = require('serve-static');
const _ = require('lodash');
const express = require('express');
const {bold} = require('chalk');

const Logger = require('./Logger');
Expand Down Expand Up @@ -48,23 +48,33 @@ async function startServer(bundleStats, opts) {

if (!chartData) return;

const app = express();
app.use(express.static(`${projectRoot}/public`));

app.get('/', (req, res) => {
res.writeHead(200, {'Content-Type': 'text/html'});
const html = renderViewer({
mode: 'server',
title: resolveTitle(reportTitle),
chartData,
defaultSizes,
enableWebSocket: true
});
return res.end(html);
const serveStaticMiddleware = serveStatic(`${projectRoot}/public`);

const server = http.createServer((req, res) => {
if (req.method === 'GET' && req.url === '/') {
const html = renderViewer({
mode: 'server',
title: resolveTitle(reportTitle),
chartData,
defaultSizes,
enableWebSocket: true
});
res.writeHead(200, {'Content-Type': 'text/html'});
res.end(html);
} else {
serveStaticMiddleware(req, res, err => {
if (err) {
console.error(err.stack || err.toString());
res.writeHead(500);
res.end();
} else {
res.writeHead(404);
th0r marked this conversation as resolved.
Show resolved Hide resolved
res.end();
}
});
}
});

const server = http.createServer(app);

await new Promise(resolve => {
server.listen(port, host, () => {
resolve();
Expand Down