neondatabase / serverless

Connect to Neon PostgreSQL from serverless/worker/edge functions
https://www.npmjs.com/package/@neondatabase/serverless
MIT License
321 stars 13 forks source link

wsAddr has a hard-coded `/v1` #3

Closed greyscaled closed 1 year ago

greyscaled commented 1 year ago

👋🏻 Hi there, really cool package, I just noticed the below in the code, I'd be happy to help contrib it.

Steps to reproduce

I was trying to run this in a browser with a custom proxy, and for reasons related to #1 , I wasn't fully successful. That said, I am pretty sure this would be the repro:

import { Client, neonConfig } from '@neondatabase/serverless';

neonConfig.wsProxy = 'my-service/ws/proxy';

async function whatsTheTimeMrPostgres() {
  const client = new Client(env.DATABASE_URL);
  await client.connect();
  const { rows: [{ now }] } = await client.query('select now();');
  await client.end();
  return now;
}

Expected result

The ws proxy address that would be called is:

ws://my-service/ws/proxy?address=host:port

Actual result

ws://my-service/ws/proxy/v1?address=host:port

I think this is just a matter of a hard-code. I know the no-webassembly branch is under heavy dev, so I'm going to permalink from that branch instead of main:

https://github.com/neondatabase/serverless/blob/f8ef1f4e48d571d53186e05456892e6cdf532128/shims/net/index.ts#L74

I don't know if this would break an internal use case for you, but I wonder if it could be as simple as just adding the /v1 here:

https://github.com/neondatabase/serverless/blob/f8ef1f4e48d571d53186e05456892e6cdf532128/shims/net/index.ts#L48

Like so:

static wsProxy: string | ((host: string) => string) = 'ws.manipulexity.com/v1';

Open to other alternatives or comments, happy to contrib/help against main if you'd like 😄.

Thanks!

Environment

Logs, links

greyscaled commented 1 year ago

I have a commit that I could open as a PR, but will wait for reply before doing so as to keep noise down:

https://github.com/neondatabase/serverless/compare/main...greyscaled:serverless:main

jawj commented 1 year ago

Thanks, that's a good catch (which had occurred to me at some point, but I'm not sure if or when I'd have got round to changing it).

I'll make sure we move the /v1 out to the config string in the next release.

jawj commented 1 year ago

This is now done in the no-websockets branch.

jawj commented 1 year ago

... and that's now in main and released as of npm package version 0.2.0.