Align arguments of `connect()` in node:tls and node:net to Node (#10854)

henrikstorck commented 1 week ago

What does this PR do?

The handling of arguments in Bun's implementation of connect() (node:tls) deviates from the available combinations of arguments in Node. This breaks compatibility for many packages that rely on the order of parameters in Node.

I adjusted the connect method to allow the same amount and order of arguments as Node.

[X] Code changes

How did you verify your code works?

[X] I included a test for the new code, or existing tests cover it
[X] I ran my tests locally and they pass (bun-debug test test-file-name.test)

github-actions[bot] commented 1 week ago

❌ @henrikstorck, your commit has failing tests :(

💪 3 failing tests Darwin AARCH64

test/cli/install/bun-install.test.ts 1 failing
test/js/bun/http/serve.test.ts 1 failing
test/js/node/http2/node-http2.test.js 1 failing

💻 4 failing tests Darwin x64 baseline

test/cli/install/bunx.test.ts 1 failing
test/cli/install/registry/bun-install-registry.test.ts 2 failing
test/js/bun/http/serve.test.ts 1 failing
test/js/web/workers/worker.test.ts 1 failing

💻 5 failing tests Darwin x64

test/cli/install/bun-create.test.ts 2 failing
test/cli/install/registry/bun-install-registry.test.ts 2 failing
test/js/bun/http/serve.test.ts 2 failing
test/js/bun/spawn/spawn-kill-signal.test.ts 1 failing
test/js/bun/test/test-test.test.ts 1 failing

🪟💻 8 failing tests Windows x64 baseline

test/cli/install/bunx.test.ts 1 failing
test/cli/test/bun-test.test.ts 1 failing
test/integration/next-pages/test/dev-server-ssr-100.test.ts 1 failing
test/integration/next-pages/test/dev-server.test.ts 1 failing
test/integration/next-pages/test/next-build.test.ts 1 failing
test/js/bun/shell/bunshell.test.ts 1 failing
test/js/bun/spawn/spawn.test.ts 1 failing
test/js/node/dns/node-dns.test.js 2 failing

🪟💻 10 failing tests Windows x64

test/cli/install/bunx.test.ts 1 failing
test/cli/install/registry/bun-install-registry.test.ts 2 failing
test/integration/esbuild/esbuild.test.ts 1 failing
test/integration/next-pages/test/dev-server-ssr-100.test.ts 1 failing
test/integration/next-pages/test/dev-server.test.ts 1 failing
test/integration/next-pages/test/next-build.test.ts 1 failing
test/js/bun/shell/bunshell.test.ts 1 failing
test/js/bun/shell/commands/rm.test.ts 1 failing
test/js/bun/spawn/spawn.test.ts 1 failing
test/js/node/dns/node-dns.test.js 2 failing

View logs

henrikstorck commented 1 week ago

Might fix some of the following issues: #10711 #7851 https://github.com/oven-sh/bun/issues/6681 https://github.com/oven-sh/bun/issues/4540 https://github.com/oven-sh/bun/issues/4136 https://github.com/oven-sh/bun/issues/4033 https://github.com/oven-sh/bun/issues/3701 https://github.com/oven-sh/bun/issues/3170 https://github.com/taskforcesh/bullmq/issues/2237

Maybe related to this but unlikely: https://github.com/oven-sh/bun/issues/5147

Jarred-Sumner commented 1 week ago

Thank you for this. Looks like there are some failing node:tls tests. Our tests might be wrong and need to be updated.

henrikstorck commented 1 week ago

Thank you for this. Looks like there are some failing node:tls tests. Our tests might be wrong and need to be updated.

Do you mean the github-actions message from 20h ago?

I pushed a few commits since then. Should be a lot less failing tests now

henrikstorck commented 1 week ago

@Jarred-Sumner It just finished re-running the tests and no more failing node:tls tests I think

nektro commented 1 week ago

this appears to have broken test/js/third_party/postgres/postgres.test.ts on all platforms and is now showing up in other PRs. reverting the commit locally fixes it. the failure did not show up here because tests were skipped since this came from a contribution and the TLS_POSTGRES_DATABASE_URL was thus not exposed to the CI environment.

nektro commented 1 week ago

Going to revert this and revisit. Apologies!

henrikstorck commented 1 week ago

@nektro Sorry! Thank you for noticing though.

Are there any other tests failing because of this? Would like to make sure they all pass again before recreating the PR

nektro commented 1 week ago

no worries! thank u for the patch :)

that looked to be like the only one. all the new ones you added worked. I'm working on making a new PR to test all the changes and will add you as co-author

henrikstorck commented 1 week ago

Hey @nektro, were you able to reproduce the failing tests locally? When I use my local db and run the test, they pass. Not sure what I'm doing wrong. Thanks in advance.

nektro commented 1 week ago

yeah with https://github.com/oven-sh/bun/commit/6217d78567d89fbd791381388c8333e2687efcb8 checked out and making a fresh build the Postgres test file was timing out for me locally

oven-sh / bun