Allow `markbind serve` to specify custom host #2382 by jflam1301 · Pull Request #2395 · MarkBind/markbind

jflam1301 · 2024-01-28T07:40:24Z

What is the purpose of this pull request?

Overview of changes:
Resolves #2382
Added -a option to the serve command

Anything you'd like to highlight/discuss:
Default host/address is 127.0.0.1. If the custom host specified by users is not working, run server on the default host

Testing instructions:
Run markbind serve -a <address>, eg. markbind serve -a 127.0.0.1

Proposed commit message: (wrap lines at 72 characters)
Allow markbind serve to specify custom host

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

codecov · 2024-01-28T07:44:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5ca4704) 48.87% compared to head (1cedfbe) 48.87%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2395   +/-   ##
=======================================
  Coverage   48.87%   48.87%           
=======================================
  Files         124      124           
  Lines        5238     5238           
  Branches     1109     1109           
=======================================
  Hits         2560     2560           
  Misses       2371     2371           
  Partials      307      307

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yucheng11122017

Hi @LamJiuFong thanks for the work!
One thing is I think it might be better to keep 127.0.0.1 as the default if the customised port isn't valid. What do you think?

yucheng11122017 · 2024-01-28T09:35:57Z

+    } else if (e.code === 'EADDRNOTAVAIL') {
+      console.log('address(host) %s is not available. Trying another address'.yellow, host);
+      setTimeout(function() {
+        server.listen(port, '0.0.0.0');


I'll prefer to make 127.0.0.1 as the default address if the host is invalid. Because 0.0.0.0 has some special meaning and might accidentally expose it to the rest of the interner (see link).

yucheng11122017 · 2024-01-28T09:36:41Z

  .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 localhost')


Forgot a closing bracket here 😔

lhw-1

The functional code should be good to go once you've addressed the comments - remember to update the relevant documentation and tests afterwards.

lhw-1 · 2024-01-28T14:05:50Z

  .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 localhost')


Instead of "localhost", let's specify the address as 127.0.0.1.

lhw-1 · 2024-01-28T14:15:51Z

      }, 1000);
-    } else {
+    } else if (e.code === 'EADDRNOTAVAIL') {
+      console.log('address(host) %s is not available. Trying another address'.yellow, host);


Suggested change

console.log('address(host) %s is not available. Trying another address'.yellow, host);

console.log('%s is not available. Trying another address.'.yellow, host);

jflam1301 · 2024-01-29T04:44:42Z

Hi @yucheng11122017 @lhw-1, thank you for the suggestions. Initially I was thinking of setting 0.0.0.0 as the default host so that the users can run the server once there is any available host. However, I did overlook the safety issues of allowing this to happen. I think it's good to set '127.0.0.1' as the default host.

I will work on the descriptions too.

itsyme · 2024-02-01T07:51:56Z

Do you think it is useful to add a warning for 0.0.0.0 specifically? I feel this way as our target audience are not only for technical people and could potentially unknowingly open up dangers to our non-technical users

yucheng11122017 · 2024-02-01T07:52:21Z

Do you think it is useful to add a warning for 0.0.0.0 specifically? I feel this way as our target audience are not only for technical people and could potentially unknowingly open up dangers to our non-technical users

Yeah this sounds good!

yucheng11122017

Hi @LamJiuFong thanks for the changes! Last thing is to add a warning as suggested by @itsyme when 0.0.0.0 is used. :)

yucheng11122017 · 2024-02-06T13:46:19Z

 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';


Suggested change

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

var host = options.host || '127.0.0.1';

jflam1301 · 2024-02-08T18:07:18Z

Would like to seek advice on the latest commit, I display a warning and prompt the user to input "y" in the CLI to proceed with host 0.0.0.0 at their own risk. Is this a good idea to warn the users?

itsyme · 2024-02-10T02:33:41Z

        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;


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

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?

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!

itsyme · 2024-02-13T12:49:37Z

  options = options || {};
-  var host = options.host || '0.0.0.0';
+  var host = options.host !== undefined ? options.host : '127.0.0.1';
  var port = options.port !== undefined ? options.port : 8080; // 0 means random


Suggested change

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

var port = options.port ?? 8080; // 0 means random

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';

jflam1301 · 2024-02-14T13:29:56Z

@yucheng11122017 @itsyme
I researched on the difference between the null coalescing operator ?? and the OR operator || and found out that the main difference between them is during the handling of falsy values like 0, empty string, empty list etc.
eg. 0 ?? 100 will return 0 but 0 || 100 will return 100.

I was thinking if we should use the ?? operator while handling missing host and port (line 161-162) so that if the user specifically enter 0, markbind will not default it to a specific host/port defined by us.
eg. using 0 || 127.0.0.1 will set the host to be 127.0.0.1 but 0 ?? 127.0.0.1 will set the host to be 0.0.0.0
I feel like the latter is more suitable. What do you think?

kaixin-hc · 2024-02-14T15:15:56Z

Sounds good to me :) do make the change, I think this PR is almost there!

itsyme · 2024-02-15T01:25:28Z

@yucheng11122017 @itsyme I researched on the difference between the null coalescing operator ?? and the OR operator || and found out that the main difference between them is during the handling of falsy values like 0, empty string, empty list etc. eg. 0 ?? 100 will return 0 but 0 || 100 will return 100.

I was thinking if we should use the ?? operator while handling missing host and port (line 161-162) so that if the user specifically enter 0, markbind will not default it to a specific host/port defined by us. eg. using 0 || 127.0.0.1 will set the host to be 127.0.0.1 but 0 ?? 127.0.0.1 will set the host to be 0.0.0.0 I feel like the latter is more suitable. What do you think?

I agree! My opinion is that we should use the nullish coalescing operator ?? in this case as this line should only be handling the responsibility of checking if any input is present from the user. For the other cases (e.g. 0, empty string, etc.), the responsibility should lie with the validation function instead. Wdyt @yucheng11122017 ?

yucheng11122017 · 2024-02-15T09:46:25Z

@yucheng11122017 @itsyme I researched on the difference between the null coalescing operator ?? and the OR operator || and found out that the main difference between them is during the handling of falsy values like 0, empty string, empty list etc. eg. 0 ?? 100 will return 0 but 0 || 100 will return 100.
I was thinking if we should use the ?? operator while handling missing host and port (line 161-162) so that if the user specifically enter 0, markbind will not default it to a specific host/port defined by us. eg. using 0 || 127.0.0.1 will set the host to be 127.0.0.1 but 0 ?? 127.0.0.1 will set the host to be 0.0.0.0 I feel like the latter is more suitable. What do you think?

I agree! My opinion is that we should use the nullish coalescing operator ?? in this case as this line should only be handling the responsibility of checking if any input is present from the user. For the other cases (e.g. 0, empty string, etc.), the responsibility should lie with the validation function instead. Wdyt @yucheng11122017 ?

Sounds good! Thanks for the research. Remember to write it in your knowledge base to get some credit HAHA

yucheng11122017

LGTM! Thanks for the work @LamJiuFong

itsyme

LGTM! Good work @LamJiuFong!

yucheng11122017 · 2024-02-24T08:47:49Z

@all-contributors please add @LamJiuFong for code

allcontributors · 2024-02-24T08:47:51Z

@yucheng11122017

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

yucheng11122017 · 2024-02-24T08:50:15Z

@all-contributors please add @LamJiuFong for code

allcontributors · 2024-02-24T08:50:25Z

@yucheng11122017

I've put up a pull request to add @LamJiuFong! 🎉

Jiu Fong Lam added 2 commits January 28, 2024 15:01

Add -a option to 'markbind serve' command to allow custom host

71f7719

Merge branch 'master' into 2382-allow-custom-host

4b18adf

yucheng11122017 reviewed Jan 28, 2024

View reviewed changes

lhw-1 reviewed Jan 28, 2024

View reviewed changes

Jiu Fong Lam added 2 commits February 5, 2024 12:16

Change default host from 0.0.0.0 to 127.0.0.1

ed888f8

Merge branch 'master' into 2382-allow-custom-host

8f28bb4

yucheng11122017 reviewed Feb 6, 2024

View reviewed changes

Add warning and get user's confirmation on using address 0.0.0.0

d2a1d04

Jiu Fong Lam added 2 commits February 9, 2024 02:19

Remove console.log and update warning message

401bc37

Update User Guide to add option -a <address>

813a0e2

itsyme reviewed Feb 9, 2024

View reviewed changes

Comment thread packages/cli/src/cmd/serve.js

Jiu Fong Lam added 2 commits February 10, 2024 09:38

Change warning message

43cd61f

Fix warning message

fa78840

itsyme reviewed Feb 10, 2024

View reviewed changes

itsyme mentioned this pull request Feb 13, 2024

Validation when specifying custom host #2417

Closed

itsyme reviewed Feb 13, 2024

View reviewed changes

Jiu Fong Lam added 2 commits February 15, 2024 13:25

Use nullish coalesce operator to handle null or undefined host/post

30f60a9

Use regex to capture common expressions of initialising host 0.0.0.0

b602481

Merge branch 'master' into 2382-allow-custom-host

1cedfbe

yucheng11122017 approved these changes Feb 17, 2024

View reviewed changes

yucheng11122017 requested a review from itsyme February 17, 2024 06:49

itsyme approved these changes Feb 17, 2024

View reviewed changes

kaixin-hc merged commit 2647090 into MarkBind:master Feb 17, 2024

jflam1301 changed the title ~~[Request for early review on functional code] Allow markbind serve to specify custom host #2382~~ Allow markbind serve to specify custom host #2382 Feb 19, 2024

yucheng11122017 added the r.Minor Version resolver: increment by 0.1.0 label Feb 24, 2024

allcontributors Bot mentioned this pull request Feb 24, 2024

add LamJiuFong as a contributor for code #2433

Merged

jayasting98 mentioned this pull request Mar 1, 2024

Fix failing GitHub action nus-cs3281/2024#43

Merged

	console.log('address(host) %s is not available. Trying another address'.yellow, host);
	console.log('%s is not available. Trying another address.'.yellow, host);

	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
	var port = options.port ?? 8080; // 0 means random

Uh oh!

Conversation

jflam1301 commented Jan 28, 2024 • edited by yucheng11122017 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yucheng11122017 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yucheng11122017 Jan 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhw-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jflam1301 commented Jan 29, 2024

Uh oh!

itsyme commented Feb 1, 2024

Uh oh!

yucheng11122017 commented Feb 1, 2024

Uh oh!

yucheng11122017 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jflam1301 commented Feb 8, 2024

Uh oh!

Uh oh!

itsyme Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jflam1301 commented Feb 14, 2024

Uh oh!

kaixin-hc commented Feb 14, 2024

Uh oh!

itsyme commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yucheng11122017 commented Feb 15, 2024

Uh oh!

yucheng11122017 left a comment

Choose a reason for hiding this comment

Uh oh!

itsyme left a comment

Choose a reason for hiding this comment

Uh oh!

yucheng11122017 commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allcontributors Bot commented Feb 24, 2024

Uh oh!

yucheng11122017 commented Feb 24, 2024

Uh oh!

allcontributors Bot commented Feb 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jflam1301 commented Jan 28, 2024 •

edited by yucheng11122017

Loading

codecov Bot commented Jan 28, 2024 •

edited

Loading

yucheng11122017 Jan 28, 2024 •

edited

Loading

itsyme Feb 10, 2024 •

edited

Loading

itsyme commented Feb 15, 2024 •

edited

Loading

yucheng11122017 commented Feb 24, 2024 •

edited

Loading