feat: unix domain sockets and windows named pipe support #104

Mastercuber · 2023-08-14T00:51:16Z

🔗 Linked issue

Closes #1

❓ Type of change

📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Here is yet another hopefully promising PR 😁

These PR adds support for listening on unix domain sockets or on windows named pipes. It adds the assignable CLI options, copies the full socket/pipe path to clipboard if wanted, prints the socket path and ignores all HTTPS options and also the --open option.

While developing this PR, I recognized, that the close() function is neither called when the terminal is closed, nor when the process gets interrupted with Ctrl + C, so I added 3 signal traps to trigger the close call. With these traps the socket will get automatically cleaned up; without these traps - and without proper termination - the socket needs to be deleted manually before starting listhen again.

2 tests are included. Unlike the h3 app test, the handle test is returning the same result for non existing paths. Is this expected behavior?

I've successfully executed the tests on Manjaro and on Windows 10.

📝 Checklist

I have linked an issue or discussion.
I have updated the documentation accordingly.

codecov · 2023-08-14T00:52:22Z

Codecov Report

Attention: Patch coverage is 59.25926% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 50.57%. Comparing base (d115045) to head (0b06e49).
Report is 14 commits behind head on main.

Files	Patch %	Lines
src/listen.ts	64.51%	11 Missing ⚠️
src/cli.ts	0.00%	6 Missing ⚠️
src/_utils.ts	61.53%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
+ Coverage   49.42%   50.57%   +1.14%     
==========================================
  Files          17       17              
  Lines        1819     1835      +16     
  Branches      147      156       +9     
==========================================
+ Hits          899      928      +29     
+ Misses        915      902      -13     
  Partials        5        5

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

src/cli.ts

src/listen.ts

README.md

Mastercuber · 2023-08-14T18:40:50Z

Thanks for your review BTW!

Mastercuber · 2023-08-16T21:10:09Z

@pi0
After a bit of refactoring, this PR looks much cleaner now.

Adding the signals traps is separated into #108 and an issue #107 was created for it.

The getURLs function now uses getURL also for sockets and just returns earlier.

Assertions used in both tests (http and https version) are extracted into functions.

Also the docs looks a bit cleaner now. With --ipc [name] and Default: false [listhen] it should be quickly possible to understand what is meant with it.

pi0 · 2023-09-08T23:27:38Z

Thanks for putting efforts on this PR. It looks nice. I have added few refactors to simplify impl. Probably need more time to resolve merge conflicts and add little more fixes (tests are broken on macOS as it uses a random temp dir also we shall support absolute paths for socket that are outside of /tmp).

Feel free to rebase or make any of those changes if had time to spend and have a nice weekend!

Mastercuber · 2023-09-09T23:48:06Z

Indeed the generator wasn't necessary for passing the args.. I somehow have overseen the possibility to pass an object to server.listen.

Absolute paths on unixoid systems are also supported now and full pipe paths. Actually supporting full pipe paths seems unnecessary since they have to start with \\?\pipe\<pipe-name> and are therefore no actual system paths.

Also have a nice weekend 🏖️

Cheaterman

Support for relative socket paths would be nice, unless there's really a good reason to avoid that.

Cheaterman · 2023-10-11T16:49:41Z

src/_utils.ts

+    }
+    return `\\\\?\\pipe\\${_name}`;
+  }
+  return isAbsolute(_name) ? _name : join(tmpdir(), `${_name}.socket`);


Hi and sorry to already comment on unmerged code but I really need this here so am doing all sorts of hacks to make it happen. Any reason you're forcing relative socket paths to /tmp/? I use relative socket paths here and it breaks. Thanks in advance!

I've developed this feature on a linux system and have no macOS device to test it on; that's the only reason, why relative paths wasn't correctly supported. Now relative paths should also be supported on macOS. This issues description should clarify.

The tmpdir() function is returning a symbolic link on macOS, but with the realpathSync() function it's possible to resolve the link to the actual absolute path.

Hi and thanks for your hard work! I'm sorry but I feel like there might have been a bit of a misunderstanding... my question was specifically about not forcing relative paths into /tmp, instead relying on current working directory (as was my initial expectation). Is there any particular reason you force the socket into /tmp? Or would it be OK to simply change L95 to return _name instead of prepending /tmp and adding .socket ?

I thought of a path which will be the same on all OS's. The /tmp directory (or better said the os.tmpdir() function) seems to fit to this purpose. An absolute path can still be passed to through the listhen arguments.

Just returning the path _name. appending .socket and using the working directory is another possibility, but you can also pass the working directory as an argument. The /tmp directory is just used when no socket path is provided.

When this lines are commented out, the relative path utils test will fail.

I believe that's not what most people would expect :-) my use-case calls for ./frontend_run/nuxt.sock ; I can indeed use an absolute path by manually prefixing with $PWD but there's not much I can do to remove the .socket you're adding. Why wouldn't the path be the same on all OSes if you just let it through as-is? If I request frontend_run/nuxt.sock I expect to get exactly that on UNIX ($PWD/frontend_run/nuxt.sock) and something similar on Windows (%CD%\frontend_run\nuxt.sock).

EDIT: Having read the tests, I'd argue they also need to be fixed.

Thank you for your objection!

I've changed the getSocketPath function to pass the path through on unixoid systems.

Can't tell what was on my mind, while deciding to use the /tmp directory instead of passing it through to let CWD be prepended by createServer...

The tests are also fixed for this new behavior (and additionally the windows tests are separated in an extra suite)

.sock now gets only appended to the path if no path is given => listhen.sock

I think it's really good now, thanks again for all your hard work :-)

Cheaterman · 2023-10-17T10:26:37Z

Thanks again for all your hard work @Mastercuber, IMHO this PR looks really good now :-) would be great to merge it so we can have UNIX socket support for Nuxt devserver!

…./ if not present

pi0 reviewed Aug 14, 2023

View reviewed changes

src/cli.ts Outdated Show resolved Hide resolved

src/listen.ts Outdated Show resolved Hide resolved

src/listen.ts Outdated Show resolved Hide resolved

src/listen.ts Outdated Show resolved Hide resolved

Mastercuber marked this pull request as draft August 14, 2023 17:24

Mastercuber marked this pull request as ready for review August 14, 2023 18:34

Mastercuber commented Aug 14, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

Mastercuber force-pushed the ipc-listhen branch 2 times, most recently from 25a6b98 to 1148009 Compare August 16, 2023 19:54

Mastercuber force-pushed the ipc-listhen branch 6 times, most recently from c86ad62 to 93483dc Compare August 20, 2023 04:00

Mastercuber force-pushed the ipc-listhen branch from e7ea74b to 075378c Compare September 9, 2023 23:30

Mastercuber force-pushed the ipc-listhen branch 3 times, most recently from 93aeffc to 6675c02 Compare September 18, 2023 20:35

Cheaterman reviewed Oct 11, 2023

View reviewed changes

Cheaterman mentioned this pull request Oct 11, 2023

More listening options for nitro server target nuxt/nuxt#11973

Closed

3 tasks

Mastercuber force-pushed the ipc-listhen branch from 6675c02 to b8ef59f Compare October 11, 2023 19:20

Mastercuber and others added 5 commits March 22, 2024 19:34

feat: unix domain socket and windows named pipe support

d72e391

test: http/https connection to socket created with handle/h3App

795fb57

indicating if https or http is used with a domain socket

32da9b9

use default tmp directory for constructing socket path

7e534cc

small refactors

dc5b92c

Mastercuber and others added 9 commits March 22, 2024 19:37

add/remove lost changes (rebase)

ea07d22

fix: expected url in tests

4b043a7

feat: also support absolute socket and full pipe paths

be6ee87

test: absolute socket and full pipe paths

5664eae

fix: support relative paths for socket also on macOS

20c892e

removes prepending tmp directory and appending .socket

01620a5

test: splits windows test in separate suite

07f097b

make clear, that the socket/pipe is created in the CWD by prepending …

bd79c01

…./ if not present

chore: apply automated lint fixes

50225e0

Mastercuber force-pushed the ipc-listhen branch from cbda020 to 50225e0 Compare March 22, 2024 18:42

Mastercuber added 2 commits March 22, 2024 19:50

fix lost comment start

cdaafbe

fix: ipc-tests

0b06e49

Mastercuber force-pushed the ipc-listhen branch from 7db542a to 0b06e49 Compare March 22, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unix domain sockets and windows named pipe support #104

feat: unix domain sockets and windows named pipe support #104

Mastercuber commented Aug 14, 2023

codecov bot commented Aug 14, 2023 •

edited

Loading

Mastercuber commented Aug 14, 2023 •

edited

Loading

Mastercuber commented Aug 16, 2023 •

edited

Loading

pi0 commented Sep 8, 2023

Mastercuber commented Sep 9, 2023 •

edited

Loading

Cheaterman left a comment

Cheaterman Oct 11, 2023

Mastercuber Oct 11, 2023

Cheaterman Oct 12, 2023

Mastercuber Oct 12, 2023 •

edited

Loading

Cheaterman Oct 16, 2023 •

edited

Loading

Mastercuber Oct 16, 2023 •

edited

Loading

Cheaterman Oct 17, 2023

Cheaterman commented Oct 17, 2023

feat: unix domain sockets and windows named pipe support #104

Are you sure you want to change the base?

feat: unix domain sockets and windows named pipe support #104

Conversation

Mastercuber commented Aug 14, 2023

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

codecov bot commented Aug 14, 2023 • edited Loading

Codecov Report

Mastercuber commented Aug 14, 2023 • edited Loading

Mastercuber commented Aug 16, 2023 • edited Loading

pi0 commented Sep 8, 2023

Mastercuber commented Sep 9, 2023 • edited Loading

Cheaterman left a comment

Choose a reason for hiding this comment

Cheaterman Oct 11, 2023

Choose a reason for hiding this comment

Mastercuber Oct 11, 2023

Choose a reason for hiding this comment

Cheaterman Oct 12, 2023

Choose a reason for hiding this comment

Mastercuber Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Cheaterman Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Mastercuber Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Cheaterman Oct 17, 2023

Choose a reason for hiding this comment

Cheaterman commented Oct 17, 2023

codecov bot commented Aug 14, 2023 •

edited

Loading

Mastercuber commented Aug 14, 2023 •

edited

Loading

Mastercuber commented Aug 16, 2023 •

edited

Loading

Mastercuber commented Sep 9, 2023 •

edited

Loading

Mastercuber Oct 12, 2023 •

edited

Loading

Cheaterman Oct 16, 2023 •

edited

Loading

Mastercuber Oct 16, 2023 •

edited

Loading