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.

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

Fix printing interfaces on node18 #811

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

Conversation

alexanderankin
Copy link

@alexanderankin alexanderankin commented May 9, 2022

Changes minor logging output issue

Relevant issues

Fixes #810

Contributor checklist
  • Provide tests for the changes (unless documentation-only)
  • Documented any new features, CLI switches, etc. (if applicable)
    • Server --help output
    • README.md
    • doc/http-server.1 (use the same format as other entries)
  • The pull request is being made against the master branch
Maintainer checklist
  • Assign a version triage tag
  • Approve tests if applicable

@lygstate
Copy link

better to testing with nodejs 18

Copy link

@dvtate dvtate left a comment

Choose a reason for hiding this comment

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

lgtm, although I'd just add a check for || details.family === 4 on 229 for simplicity

@alexanderankin
Copy link
Author

I would say that this adds clarity right -- code is not just read by computers but also by humans, and node 16 will be EOL at some point, and then this could simply be removed again (but only if its clear that it is a node <= 18 thing). I believe I've granted the maintainers access to the branch, so they can make simplifications as they see fit before merging.

@thornjad
Copy link
Member

Thank you for the PR! And for the write access! I just added a small comment. I'm going to add Node 18 testing to master, merge that in, let tests run, then merge

@thornjad
Copy link
Member

It would actually be much easier for you to merge in latest master than me, could you whenever you get a chance?

@alexanderankin
Copy link
Author

alexanderankin commented May 31, 2022 via email

@alexanderankin
Copy link
Author

merged and pushed

@alexanderankin
Copy link
Author

@thornjad - its merged, in case you missed it

@alexanderankin
Copy link
Author

@thornjad - master has been merged

@dvtate
Copy link

dvtate commented Jun 16, 2022

@thornjad it's been over a month,,,;.;

@dvtate
Copy link

dvtate commented Aug 11, 2022

Maintainer is asking for funding but hasn't touched this repo all summer (~3 months)

@dvtate
Copy link

dvtate commented Oct 13, 2022

It seems that this PR is now only needed to support nodejs v18.0.0 - v18.4.0 as they've reverted the change. Would update the version numberrs in the pr.
Screenshot-20221013172345-509x295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Printing interfaces filters incorrectly on node 18 (in server listening message)
4 participants