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

Fix printing interfaces on node18

Open alexanderankin opened this issue 2 years ago • 11 comments

Changes minor logging output issue

Relevant issues

Fixes #810

Contributor checklist
  • [x] Provide tests for the changes (unless documentation-only)
  • [x] 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)
  • [x] The pull request is being made against the master branch
Maintainer checklist
  • [ ] Assign a version triage tag
  • [ ] Approve tests if applicable

alexanderankin avatar May 09 '22 07:05 alexanderankin

better to testing with nodejs 18

lygstate avatar May 17 '22 20:05 lygstate

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.

alexanderankin avatar May 26 '22 16:05 alexanderankin

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 avatar May 31 '22 21:05 thornjad

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

thornjad avatar May 31 '22 22:05 thornjad

Should be able to do that tonight

On Tue, May 31, 2022, 6:09 PM Jade Michael Thornton < @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/http-party/http-server/pull/811#issuecomment-1142685778, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACECGJE5R2VQ46W2C3L3CGLVM2E2ZANCNFSM5VNJVFOQ . You are receiving this because you authored the thread.Message ID: @.***>

alexanderankin avatar May 31 '22 22:05 alexanderankin

merged and pushed

alexanderankin avatar May 31 '22 22:05 alexanderankin

@thornjad - its merged, in case you missed it

alexanderankin avatar Jun 01 '22 00:06 alexanderankin

@thornjad - master has been merged

alexanderankin avatar Jun 07 '22 13:06 alexanderankin

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

dvtate avatar Jun 16 '22 22:06 dvtate

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

dvtate avatar Aug 11 '22 14:08 dvtate

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

dvtate avatar Oct 13 '22 22:10 dvtate