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

Jump to bottom

Added QR Code for public IP #738

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Added QR Code for public IP #738

wants to merge 13 commits into from

Conversation

HARDY8118
Copy link

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.

@thornjad
Copy link
Member

thornjad commented Oct 5, 2021

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?

@HARDY8118
Copy link
Author

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
Copy link
Author

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.

@thornjad
Copy link
Member

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 thornjad added the minor version non-breaking, non-trivial change label Oct 13, 2021
thornjad
thornjad requested changes Oct 13, 2021
View reviewed changes
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

We still need to add the new option to doc/http-sever.1 as well

@thornjad
Copy link
Member

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

thornjad
thornjad requested changes Oct 15, 2021
View reviewed changes
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

Sorry, looks like another package-lock conflict. npm is going crazy changing that differently for everyone lately, so every merge seems to conflict with something

HARDY8118 and others added 2 commits October 15, 2021 23:51
@HARDY8118
Changed scope for plainIP
49a56ad 
Fixed failing tests

Removed -Q option for showQR option
@HARDY8118
Merge branch 'master' into qr
d5af101
thornjad
thornjad requested changes Oct 18, 2021
View reviewed changes
bin/http-server
@@ -227,6 +229,7 @@ function listen(port) {
Object.keys(ifaces).forEach(function (dev) {
ifaces[dev].forEach(function (details) {
if (details.family === 'IPv4') {
plainIp = protocol + details.address + ':' + port.toString()
Copy link
Member

Choose a reason for hiding this comment

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

I just realized, this is inside a loop! So it'll only be making a code for the last IP, which is usually the public IP, but I don't believe that's guaranteed. Some thoughts, and I welcome yours:

  • We could make a code for each IP (would get to be a lot)
  • We could print out which IP is linked next to/under/above the QR code as a label
  • I don't even know what else right now, I'm sure there are more options

Copy link
Author

Choose a reason for hiding this comment

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

I am out of ideas right now.

But I like the one's you proposed

I'm all onboard with either. We can use device identifier (like --qr-code lo or --qr-code wlp3s0) or index (like --qr-code 0 for local and --qr-code 1, --qr-code 2 ... for public) if we are going with that.

Or maybe the other one where we can print IP next to QR. But I have some concerns with this one as this won't give an option for local IP and if we try to make QR for all IPs that will fill up the screen quickly. Personally I like the info as clean as it was with addition of QR optionally.

What are your suggestions?

@zbynek zbynek mentioned this pull request Apr 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornjad thornjad thornjad requested changes

Assignees
No one assigned
Labels
feature minor version non-breaking, non-trivial change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HARDY8118 @thornjad