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

Allow markbind serve to specify custom host #2382 #2395

Merged
merged 12 commits into from
Feb 17, 2024
Merged

Allow markbind serve to specify custom host #2382 #2395

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
71f7719
Add -a option to 'markbind serve' command to allow custom host
Jan 28, 2024
4b18adf
Merge branch 'master' into 2382-allow-custom-host
Jan 28, 2024
ed888f8
Change default host from 0.0.0.0 to 127.0.0.1
Feb 5, 2024
8f28bb4
Merge branch 'master' into 2382-allow-custom-host
Feb 5, 2024
d2a1d04
Add warning and get user's confirmation on using address 0.0.0.0
Feb 8, 2024
401bc37
Remove console.log and update warning message
Feb 8, 2024
813a0e2
Update User Guide to add option `-a <address>`
Feb 8, 2024
43cd61f
Change warning message
Feb 10, 2024
fa78840
Fix warning message
Feb 10, 2024
30f60a9
Use nullish coalesce operator to handle null or undefined host/post
Feb 15, 2024
b602481
Use regex to capture common expressions of initialising host 0.0.0.0
Feb 15, 2024
1cedfbe
Merge branch 'master' into 2382-allow-custom-host
LamJiuFong Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ program
.option('-p, --port <port>', 'port for server to listen on (Default is 8080)')
.option('-s, --site-config <file>', 'specify the site config file (default: site.json)')
.option('-d, --dev', 'development mode, enabling live & hot reload for frontend source files.')
.option('-a, --address <address>', 'specify the server address/host (Default is 127.0.0.1)')
.description('build then serve a website from a directory')
.action((userSpecifiedRoot, options) => {
serve(userSpecifiedRoot, options);
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/cmd/serve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ function serve(userSpecifiedRoot, options) {
logLevel: 0,
root: outputFolder,
port: options.port || 8080,
host: options.address || '127.0.0.1',
middleware: [],
mount: [],
};
Expand Down Expand Up @@ -111,8 +112,9 @@ function serve(userSpecifiedRoot, options) {
const server = liveServer.start(serverConfig);
server.addListener('listening', () => {
const address = server.address();
const serveHost = address.address === '0.0.0.0' ? '127.0.0.1' : address.address;
const serveURL = `http://${serveHost}:${address.port}`;
const serveHost = address.address;
Copy link
Contributor

@itsyme itsyme Feb 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about adding some validation to the user input host address?

I believe that this could be better for UX as

  • it checks the host address before building the pages (which could take some time)
  • it better handles the errors that are being thrown. Currently users will be exposed to errors like Error: getaddrinfo ENOTFOUND 127.0.300.1

I was thinking some sort of simple validation like checking if the input is an actual IP address and if any number in the address exceeds 255 could improve the user experience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, good idea! I was thinking if we want to do this validation, we can actually just do the validations (including check if port is unavailable, check if host is being used etc) all at once before building the pages. Currently, all these validations are done after building the pages.

Since this PR is focusing on adding another small feature, I suggest we create a separate issue and pull request to specifically address this validation issue. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep I think a separate issue is fine as well! I believe there is some level of error handling for when the host is being used. From my experience if I specified a used host, the nearest unused hostname will be used, which allows the user to still serve the site. I think we can discuss about what needs to be validated once the issue is opened!

const servePort = address.port;
const serveURL = `http://${serveHost}:${servePort}`;
logger.info(`Serving "${outputFolder}" at ${serveURL}`);
logger.info('Press CTRL+C to stop ...');
});
Expand Down
16 changes: 11 additions & 5 deletions packages/cli/src/lib/live-server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ function entryPoint(staticHandler, file) {

/**
* Start a live server with parameters given as an object
* @param host {string} Address to bind to (default: 0.0.0.0)
* @param host {string} Address to bind to (default: 127.0.0.1)
* @param port {number} Port number (default: 8080)
* @param root {string} Path to root directory (default: cwd)
* @param watch {array} Paths to exclusively watch for changes
Expand All @@ -158,7 +158,7 @@ function entryPoint(staticHandler, file) {
*/
LiveServer.start = function(options) {
options = options || {};
var host = options.host || '0.0.0.0';
var host = options.host !== undefined ? options.host : '127.0.0.1';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var host = options.host !== undefined ? options.host : '127.0.0.1';
var host = options.host || '127.0.0.1';

var port = options.port !== undefined ? options.port : 8080; // 0 means random
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var port = options.port !== undefined ? options.port : 8080; // 0 means random
var port = options.port ?? 8080; // 0 means random

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I wasn't aware of the difference between null and undefined in JS. Thank you for reminding.
I will also set line 161 to be
var host = options.host ?? '127.0.0.1';

var root = options.root || process.cwd();
var mount = options.mount || [];
Expand Down Expand Up @@ -281,7 +281,13 @@ LiveServer.start = function(options) {
setTimeout(function() {
server.listen(0, host);
}, 1000);
} else {
} else if (e.code === 'EADDRNOTAVAIL') {
console.log('%s is not available. Trying another address'.yellow, host);
setTimeout(function() {
server.listen(port, '127.0.0.1');
}, 1000);
}
else {
console.error(e.toString().red);
LiveServer.shutdown();
}
Expand All @@ -292,8 +298,8 @@ LiveServer.start = function(options) {
LiveServer.server = server;

var address = server.address();
var serveHost = address.address === "0.0.0.0" ? "127.0.0.1" : address.address;
var openHost = host === "0.0.0.0" ? "127.0.0.1" : host;
var serveHost = address.address;
var openHost = host;

var serveURL = protocol + '://' + serveHost + ':' + address.port;
var openURL = protocol + '://' + openHost + ':' + address.port;
Expand Down
Loading