Skip to content

Commit

Permalink
Migrate from union to connect
Browse files Browse the repository at this point in the history 
Union is not actively developed anymore, see [1].

Union provides a middleware capability similar to connect [2] supporting
buffered streams and providing some convenience APIs on top of the
middleware capability. However, the stream functionality offered by
union is already available since Node.js v0.12 [3] and Tabris CLI does
not use most of the APIs union offered.

Migrate middleware handling to connect. Opt for connect instead of
express, since Tabris CLI does not use most of the extra features
express offers and express comes with significantly more external
dependencies (currently connect 4 vs express 30).

Error handling notes:
  * connect errors do not have a status field. Only log the error
    message.
  * connect does not handle 404 as an error as union did. Restore this
    behavior by using an extra middleware to catch all unhandled
    requests and treat them as errors.

[1]: http-party/http-server#138 (comment)
[2]: https://www.npmjs.com/package/connect
[3]: flatiron/union#61 (comment)

Fix #71

Change-Id: If502571e61f53098afd6cbfbfa66e8053691eeda
  • Loading branch information
@cpetrov
cpetrov committed Jan 29, 2020
1 parent 9239980 commit dd5692d
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 39 deletions.
81 changes: 68 additions & 13 deletions package-lock.json
View file

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
Expand Up @@ -6,6 +6,7 @@
"chalk": "^1.1.3",
"cli-progress": "^1.4.1",
"commander": "^2.12.2",
"connect": "^3.7.0",
"ecstatic": "^2.2.2",
"filewatcher": "^3.0.1",
"fs-extra": "^4.0.0",
Expand All @@ -16,7 +17,6 @@
"promptly": "^2.2.0",
"qrcode": "^1.4.4",
"semver": "^5.3.0",
"union": "^0.5.0",
"update-notifier": "^2.2.0",
"ws": "^5.2.0",
"xml2js": "^0.4.17",
Expand Down
3 changes: 2 additions & 1 deletion src/services/GetFilesMiddleware.js
View file
Expand Up @@ -22,7 +22,8 @@ module.exports = class GetFilesMiddleware extends EventEmitter {
if (param !== LOCAL_FILES) {
throw new Error(`Invalid parameter ${GET_FILES}=${param}`);
}
return res.json(this._generateChunk(this._getLocalPath(req.url)));
res.setHeader('content-type', 'application/json');
return res.end(JSON.stringify(this._generateChunk(this._getLocalPath(req.url))));
}
next();
}
Expand Down
46 changes: 32 additions & 14 deletions src/services/Server.js
View file
Expand Up @@ -3,7 +3,7 @@ const {join} = require('path');
const EventEmitter = require('events');
const {readJsonSync, existsSync, lstat} = require('fs-extra');
const ecstatic = require('ecstatic');
const union = require('union');
const connect = require('connect');
const portscanner = require('portscanner');
const {red, blue} = require('chalk');
const WebSocket = require('ws');
Expand Down Expand Up @@ -149,16 +149,11 @@ module.exports = class Server extends EventEmitter {
if (!Server.externalAddresses.length) {
throw new Error('No remotely accessible network interfaces');
}
this._server = union.createServer({
before: this._createMiddlewares(appPath, main),
onError: (err, req, res) => {
this.emit('request', req, err);
res.end();
}
});
const app = connect();
this._createMiddlewares(appPath, main).forEach(middleware => app.use(middleware));
let port = this._port || await this._findAvailablePort();
return new Promise((resolve, reject) => {
this._server.listen(port, err => {
this._server = app.listen(port, err => {
if (err) {
reject(err);
}
Expand All @@ -181,24 +176,36 @@ module.exports = class Server extends EventEmitter {

_createMiddlewares(appPath, main) {
return [
this._createErrorHandler(),
this._createRequestEmitter(),
this._createGetFilesMiddleware(appPath),
this._createDeliverEmitter(),
this._createPackageJsonMiddleware(main),
this._createBootJsMiddleware(appPath),
this._createDefaultRouteMiddleware(),
ecstatic({root: appPath, showDir: false})
ecstatic({root: appPath, showDir: false}),
this._create404Handler()
];
}

_logRequest(req, err) {
if (err) {
this.terminal.error(red(`${req.method} ${req.url} ${err.status}: "${err.message || err}"`));
this.terminal.error(red(`${req.method} ${req.url}: "${err.message || err}"`));
} else {
this.terminal.info(blue(`${req.method} ${req.url}`));
}
}

_createErrorHandler() {
// Error handling middlewares have four parameters.
// The fourth parameter needs to be given although it's currently not used.
// eslint-disable-next-line no-unused-vars
return (err, req, res, next) => {
this.emit('request', req, err);
res.end();
};
}

_createRequestEmitter() {
return (req, res, next) => {
this.emit('request', req);
Expand Down Expand Up @@ -228,7 +235,8 @@ module.exports = class Server extends EventEmitter {
}
return (req, res, next) => {
if (req.url === '/package.json') {
return res.json(this._packageJson);
res.setHeader('content-type', 'application/json');
return res.end(JSON.stringify(this._packageJson));
}
next();
};
Expand All @@ -240,7 +248,8 @@ module.exports = class Server extends EventEmitter {
}
return (req, res, next) => {
if (req.url === '/node_modules/tabris/boot.min.js') {
return res.text(getBootJs(
res.setHeader('content-type', 'text/plain');
return res.end(getBootJs(
appPath,
this.debugServer.getNewSessionId(),
encodeURIComponent(this.serverId)
Expand All @@ -253,13 +262,22 @@ module.exports = class Server extends EventEmitter {
_createDefaultRouteMiddleware() {
return async (req, res, next) => {
if (req.url === '/') {
res.html(await this._html.generate());
res.setHeader('content-type', 'text/html');
res.end(await this._html.generate());
} else {
next();
}
};
}

_create404Handler() {
return (req, res) => {
res.writeHead(404, {'Content-Type': 'text/plain'});
res.end('Not found\n');
this.emit('request', req, '404: Not found');
};
}

_findAvailablePort() {
return portscanner.findAPortNotInUse(BASE_PORT, MAX_PORT, '127.0.0.1');
}
Expand Down
19 changes: 10 additions & 9 deletions test/GetFilesMiddleware.test.js
View file
Expand Up @@ -41,20 +41,21 @@ describe('GetFilesMiddleware', () => {
return eval(code);
},
load(url) {
const json = spy();
const next = spy();
const end = stub();
const setHeader = stub();
const next = stub();
try {
middleware.handleRequest({url: url.slice(1)}, {json}, next);
if (json.notCalled && next.calledOnce) {
middleware.handleRequest({url: url.slice(1)}, {end, setHeader}, next);
if (end.notCalled && next.calledOnce) {
return null;
} else if (json.calledOnce) {
return JSON.stringify(json.firstCall.args[0]);
} else if (end.calledOnce) {
return end.firstCall.args[0];
}
} catch (ex) {
console.error(ex.message);
console.error(ex.stack);
}
throw new Error(`Inconsistent handleRequest behavior ${json.callCount}/${next.callCount}`);
throw new Error(`Inconsistent handleRequest behavior ${end.callCount}/${next.callCount}`);
},
createLoader: stub().returns('orgCreateLoader'),
readJSON: stub().returns('orgReadJSON'),
Expand Down Expand Up @@ -314,7 +315,7 @@ describe('GetFilesMiddleware', () => {

expect(loader).to.be.null;
expect(middleware.handleRequest).to.have.been.calledTwice;
expect(middleware.handleRequest.getCall(0).args[1].json).to.have.been.calledOnce; // 'req.json'
expect(middleware.handleRequest.getCall(0).args[1].end).to.have.been.calledOnce; // 'req.end'
expect(middleware.handleRequest.getCall(1).args[2]).to.have.been.calledOnce; // 'next'
});

Expand All @@ -323,7 +324,7 @@ describe('GetFilesMiddleware', () => {

expect(loader).to.be.null;
expect(middleware.handleRequest).to.have.been.calledTwice;
expect(middleware.handleRequest.getCall(0).args[1].json).to.have.been.calledOnce; // 'req.json'
expect(middleware.handleRequest.getCall(0).args[1].end).to.have.been.calledOnce; // 'req.end'
expect(middleware.handleRequest.getCall(1).args[2]).to.have.been.calledOnce; // 'next'
});

Expand Down
2 changes: 1 addition & 1 deletion test/serve.test.js
View file
Expand Up @@ -253,7 +253,7 @@ describe('serve', function() {
fetch(`http://127.0.0.1:${port}/non-existent`)
]);
let log = stdout2.toString();
expect(log).to.contain('GET /non-existent 404: "Not found"');
expect(log).to.contain('GET /non-existent: "404: Not found"');
}).timeout(30000);

});
Expand Down

0 comments on commit dd5692d

Please sign in to comment.