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

Support socketPath in node:http request #9284

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

Fixes #6055
Fixes #2734

How did you verify your code works?

There is a test

Copy link
Member

@paperdave paperdave left a comment

Choose a reason for hiding this comment

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

yeah

};

if (!!$debug) {
fetchOptions.verbose = true;
Copy link
Member

Choose a reason for hiding this comment

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

i like this a lot

fetchOptions.unix = socketPath;
}

//@ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

we should not need a typescript ignore comment here. but considering the types are a mess and dont work, this can slide

@Jarred-Sumner Jarred-Sumner merged commit f6d5325 into main Mar 6, 2024
2 checks passed
@Jarred-Sumner Jarred-Sumner deleted the jarred/unix-domain-2 branch March 6, 2024 23:28
Copy link
Contributor

github-actions bot commented Mar 6, 2024

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Copy link
Contributor

github-actions bot commented Mar 6, 2024

@paperdave 1 files with test failures on bun-darwin-aarch64:

View test output

#f37f460cb0acec3b015222ab5e11329fe2787f9b

Copy link
Contributor

github-actions bot commented Mar 7, 2024

@paperdave 1 files with test failures on bun-darwin-x64:

View test output

#f37f460cb0acec3b015222ab5e11329fe2787f9b

Copy link
Contributor

github-actions bot commented Mar 7, 2024

❌🪟 @paperdave, there are 16 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\run\as-node.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\cli\test\bun-test.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\timers\performance.test.js
  • test\js\web\websocket\websocket.test.js
  • test\js\web\workers\worker.test.ts

Full Test Output

rglynn-dev added a commit to rglynn-dev/productivity-cli that referenced this pull request Mar 16, 2024
Add a simple http server and client using bun.
I wanted to use ipc or communicate over a unix socket,
but both options do not work currently with just bun.

I tried to use Bun.spawn, but that I couldn't grok how that works
in the context a long running server (especially if I want logs).
Might revisit that later since it feels like the correct approach
at the current scope. Also there seem to be issues with resource
usage of Bun.spawn currently.

Then wanted to use the Bun server to communicate over a unix socket
since the client and server will both be running in the same env.
That works, except Bun's `fetch` implementation doesn't currently
work with unix sockets (doh!). Fix has been merged on GH but not in
release yet (oven-sh/bun#9284). I could revisit
this approach on the next release of bun.

In the end I'm just using plain old http with a really simple Bun server,
no express etc since it's just overkill at this point. The upside is if
this ends up scaling at all I might have a hosted server which will need
to be http of course.
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.

socketPath issue with node:http http.request socketPath seems not working
2 participants