http-server icon indicating copy to clipboard operation
http-server copied to clipboard

Published 20 hours ago • verified publisher icon http-party

Added QR Code for public IP

Open HARDY8118 opened this issue 3 years ago • 5 comments

Please ensure that your pull request fulfills these requirements:

  • [ x] The pull request is being made against the master branch
  • [ x] Tests for the changes have been added (for bug fixes / features)

What is the purpose of this pull request? (bug fix, enhancement, new feature,...) Added a feature to display QR code public IP in the terminal for ease of sharing IP.

What changes did you make? Fetched the public IP (latter one in the IP list) and used qrcode package to display the qr in the terminal.

Provide some example code that this change will affect, if applicable:

Is there anything you'd like reviewers to focus on? Usability and ease of sharing IP to other devices in network.

Please provide testing instructions, if applicable: The library provides the QR code in string format which can be used for testing/display in terminal.

HARDY8118 avatar Oct 02 '21 13:10 HARDY8118

This is interesting, I would never have thought of this!

Some concerns:

  • I don't think this makes sense to be something that is generated by default, maybe it would be better to only generate with an option like --qr-code?
  • I don't love that qrcode depends on yargs, which is a very heavy dependency tree. Are there any other options to make a QR code?
  • Tests and documentation!
  • Perhaps the biggest question of all, do we need this? It's neat, but it adds a lot of processing and code for a feature that I'm not sure will be used much? What do you think?

thornjad avatar Oct 05 '21 22:10 thornjad

I really appreciate you taking time and reviewing this.

  • I agree that the feature can be enabled by passing options if user wants by --qr-code argument or similar and I can easily make that change.
  • I also understand your concern for making use of a package which brings a lot of dependencies along with it (since this package aims to provide the minamilist web server) and I think some neat and efficient way can be found for doing the same without using the qrcode package.
  • Unfortunately I am not very sure about the testing part and how can it be done but maybe I can work on something.
  • I personally use this package a lot and mostly for connecting my PC and my mobile device and it makes a lot more convenient for me by using the QR code instead of typing the whole URL. I also use my mobile device for testing and the URL generated is not static (Port number may change sometimes), I feel the need to generate the QR code everytime.

HARDY8118 avatar Oct 06 '21 17:10 HARDY8118

I have changed the qrcode package to qrcode-terminal which has around 87% less size. Also added support for -Q or --qr-code arguments and updated the README.md file as well as help text in http-server.js. Waiting for your response on this.

HARDY8118 avatar Oct 07 '21 13:10 HARDY8118

I like qrcode-terminal a lot better! I have some other small comments I'll add but I'm on board with getting this added!

thornjad avatar Oct 13 '21 04:10 thornjad

So, all the tests are failing in the same way, but without an obvious error message that I can see :(

thornjad avatar Oct 15 '21 16:10 thornjad