-
Notifications
You must be signed in to change notification settings - Fork 113
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
live queries #1408
base: v0.9
Are you sure you want to change the base?
live queries #1408
Conversation
packages/client/src/index.ts
Outdated
|
||
sse.onmessage = (event) => { | ||
// @ts-ignore | ||
callback(prepared.mapResult(JSON.parse(event.data), true).rows); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any error handling to consider here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, nothing blocking prerelease.
} else { | ||
return client.live( | ||
(db) => db.select().from(status), | ||
() => { | ||
_query(client.db).then(onData).catch(onError); | ||
}, | ||
onError, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly strange block lol but I follow
|
||
build.initNamespace({ isSchemaRequired: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate
if (listenConnection) { | ||
if (listenConnection.dialect === "postgres") { | ||
listenConnection.connection.release(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is listenConnection not a property of database
? They seem to be passed around together.
} catch (error) { | ||
return c.text((error as Error).message, 500); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to strip some of the details from this error object before returning, but can add this after prerelease.
}), | ||
}); | ||
} | ||
statusResult = await statusResolver.promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This waits for the next status update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
export const client = ({ db }: { db: ReadonlyDrizzle<Schema> }) => { | ||
// @ts-ignore | ||
const session: PgSession = db._.session; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean all queries will use the same connection? Seems important to use a pool here.
await sql | ||
.raw(` | ||
CREATE OR REPLACE FUNCTION ${notification} | ||
RETURNS trigger | ||
LANGUAGE plpgsql | ||
AS $$ | ||
BEGIN | ||
NOTIFY ${channel}; | ||
RETURN NULL; | ||
END; | ||
$$;`) | ||
.execute(qb.internal); | ||
|
||
await sql | ||
.raw(` | ||
CREATE OR REPLACE TRIGGER ${trigger} | ||
AFTER INSERT OR UPDATE OR DELETE | ||
ON "${preBuild.namespace}"._ponder_status | ||
FOR EACH STATEMENT | ||
EXECUTE PROCEDURE ${notification};`) | ||
.execute(qb.internal); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My instinct is that these should happen in the same spot that we create the _ponder_status
table (if not exists).
if (dialect === "postgres") { | ||
return { | ||
dialect: "postgres", | ||
connection: await (driver as PostgresDriver).internal.connect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this connection drops?
No description provided.