-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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.
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
Modernize the webserver code (part 2) #17684
Modernize the webserver code (part 2) #17684
Conversation
/botio unittest |
There's one more thing I'd suggest improving here, and that's the
It'd be much nicer if we directly passed in necessary parameters on initialization, by writing the constructor as follows: class WebServer {
constructor({ root, host, port, cacheExpirationTime }) {
this.root = root || ".";
this.host = host || "localhost";
this.port = port || 0;
this.server = null;
this.verbose = false;
this.cacheExpirationTime = cacheExpirationTime || 0;
this.disableRangeRequests = false;
this.hooks = {
GET: [crossOriginHandler],
POST: [],
};
} |
The `handler` method contained this code in two inline functions, triggered via callbacks, which made the `handler` method big and harder to read. Moreover, this code relied on variables from the outer scope, which made it harder to reason about because the inputs and outputs weren't easily visible. This commit fixes the problems by extracting the request checking code into a dedicated private method, and modernizing it to use e.g. `const`/ `let` instead of `var` and using template strings. The logic is now self-contained in a single method that can be read from top to bottom without callbacks and with comments annotating each check/section.
This commit converts `var` to `const`/`let`, gives the variables more readable names and annotates the code to make the flow clearer.
…r` ESLint rule This commit also moves the content type logic into a helper method to ever so slightly reduce duplication.
This makes the webserver configurable during instantiation rather than having to set the parameters afterwards.
f4d3b18
to
2e6fa79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, thanks a lot for improving this code!
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/2384d65977acf46/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8304b575539add5/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/2384d65977acf46/output.txt Total script time: 2.37 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/8304b575539add5/output.txt Total script time: 10.19 mins
|
The commit messages contain more details about the individual changes. This second part completes the extraction of self-contained functionality from the large handler method to make it more manageable and to get rid of using variables from the outer scope, and enables the ESLint
no-var
rule for this file.The first commit should be reviewed with the
?w=1
flag for an easier diff.