-
Notifications
You must be signed in to change notification settings - Fork 25
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
db: add db/triggers changes back in, fix issues with drizzle-orm package #4330
base: develop
Are you sure you want to change the base?
Conversation
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.
Two questions but mostly looks good to me, @dnbrwstr should probably review though
@@ -64,11 +64,6 @@ export default function ChannelScreen(props: Props) { | |||
const channelIsPending = !channel || channel.isPendingChannel; | |||
useFocusEffect( | |||
useCallback(() => { | |||
if (!channelIsPending) { |
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.
was this intentionally changed?
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, I found that keeping this in the useFocusEffect caused unnecessary syncs that would then trigger unnecessary react query invalidations through the change listener.
const params = (0, import_sql.fillPlaceholders)(this.query.params, placeholderValues ?? {}); | ||
this.logger.logQuery(this.query.sql, params); | ||
- return this.client.executeRawAsync(this.query.sql, params); | ||
+ return this.client.executeRaw(this.query.sql, params); |
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.
these need to be synchronous?
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, the async methods don't actually exist
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.
lgtm would be good to have @dnbrwstr take a look too
The issue we were seeing was caused by two problems with drizzle-orm:
executeAsync
andexecuteAsyncRaw
uses.