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

[FEATURE REQUEST] Cloudflare Workers support #1572

Open
jdanyow opened this issue Sep 9, 2023 · 3 comments
Open

[FEATURE REQUEST] Cloudflare Workers support #1572

jdanyow opened this issue Sep 9, 2023 · 3 comments

Comments

@jdanyow
Copy link
Contributor

jdanyow commented Sep 9, 2023

Is your feature request related to a problem? If so, please give a short summary of the problem and how the feature would resolve it

Recently Cloudflare announced support for TCP sockets. Accompanying the announcement was a release of the pg Postgres client using the new connect() API. The pull request adding support was brianc/node-postgres#2971, in which a new pg-cloudflare package was added and changes were made to take node built-ins like fs and crypto out of the critical path. There was a followup PR to improve bundler ergonomics: brianc/node-postgres#2978.

Would Tedious maintainers and community members support making similar changes to this package? A while back I heard from the Cloudflare team that SQL Server support was on the roadmap and there had been feelers out to the Tedious community for implementation guidance. Wondering if anything is underway or if we could use this issue to take the pulse and track an implementation plan since this does not appear to be a trivial change.

Reference Documentation/Specifications

@jdanyow
Copy link
Contributor Author

jdanyow commented Sep 10, 2023

Heard from the Cloudflare folks there's an ongoing effort to make the connect() API standard across runtimes, including Node.js.

Spec: https://github.com/wintercg/proposal-sockets-api
Node implementation: https://github.com/Ethan-Arrowood/socket (published to npm here)

There's a few more weeks of work to polish these up, and further time to improve them based on feedback, but the intent is to make it easier for libraries to start using the connect() API — even before it's provided out-of-the-box in Node.js itself.

@MichaelSun90
Copy link
Contributor

Hi @jdanyow , thanks for bring this up. We have not yet discussed this, but we will definitely keep an eye on this. If the connect() API is standard across runtimes include Nodejs in the future, the custom socket feature PR 1540may able to handle it already. However, it definitely need to be tested.
The custom socket feature allow user to use a socket and its corresponding connect function beside the one we default used in tedious.

@arthurschreiber
Copy link
Collaborator

Hey @jdanyow,

I looked at this. It seems like the connect API is basically a way to get a network socket that implements the webstreams Duplex API.

tedious is currently built on top of the networking functionality provided by the net and tls modules - we probably can switch that longterm to using the connect shim (which basically just provides web streams that tie back into nodejs' networking APIs).

It would involve some internal refactorings to switch to the web streams api, and we probably would also need to assess the performance impact of having that additional layer between tedious and the nodejs networking APIs. If you want to investigate what changes this would require, feel free to open a PR so we can have an initial discussion. If you can't invest time into this, we will probably not be able to look into this anytime soon, as we have some other higher priority items that we'd like to focus on.

@MichaelSun90 I think the naming here is a bit confusing. The custom socket feature we provide won't work with this, as the custom socket feature expects a Node.js Stream compatible stream to be returned, whereas this connect specification builds on top of web streams, which are not directly compatible.

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

No branches or pull requests

3 participants