Conversation
🦋 Changeset detectedLatest commit: feb9a17 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
More templates
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +276 B (+0.47%) Total Size: 58.5 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 1.05 kB ℹ️ View Unchanged
|
|
Would it make sense to mimic PostgreSQL's default ordering?
|
I have been thinking about this. I didn't do that because that would be a breaking change, but we could certainly do that if we want. @KyleAMathews @samwillis any opinions on this? |
|
On the api design, I'm tempted to not add it as a third arg, but make the second arg either a string for direction, or an object for extended options: interface OrderByOptions {
direction: OrderByDirection = `asc`
nulls: `first` | `last` = `first`
}
orderBy(
callback: OrderByCallback<TContext>,
directionOrOptions: OrderByDirection | OrderByOptions = `asc`
)This makes it much more clear what the options are, and makes it mush easer to extend to option in future if we want to. @kevin-dp what do you think? |
|
@samwillis agreed. Cleaner with a configuration object. I'm not sure about the union type |
|
Yeah I like avoiding a third arg too. Union seems fine to me as yeah, 99% of time people will just specify the direction. |
|
Also, whilst we're considering the orderBy signature #316 |
There was a problem hiding this comment.
Just actually testing this with my app. I get an error because my schema is an optional(), not nullable().
Using this branch, I updated my code to:
const { data: events } = useLiveQuery(
(query) => (
query
.from({ event: eventCollection })
.orderBy(
({ event }) => event.inserted_at
{ direction: 'asc', nulls: 'last' }
)
.where(({ event }) => eq(event.thread_id, threadId))
),
[threadId]
)I get this error:
react-dom-client.production.js:5788 TypeError: Cannot read properties of undefined (reading 'inserted_at')
at query.from.innerJoin.orderBy.direction (ChatArea.tsx:82:34)
at BaseQueryBuilder.orderBy (index.js:349:20)
at ChatArea.tsx:81:10
at buildQuery (index.js:582:18)
at liveQueryCollectionOptions (live-query-collection.js:9:54)
at createLiveQueryCollection (live-query-collection.js:200:21)
at useLiveQuery (useLiveQuery.js:16:33)
at ChatArea (ChatArea.tsx:73:28)
I'm not sure if we should:
- handle undefined fields / zod
foo.optional()s - not handle them and require the property but allow
foo.nullable()s
I think that the ability to support both would be ideal as people may well use optionals in their schema definitions. If not then we need to document the limitation.
|
@thruflo i'll open an issue for ordering on an optional column. It's a separate issue that's best addressed with a separate PR. |
5d86583 to
52ecb77
Compare
@asabil Since TanStack DB is backend-agnostic we decided to stick to JS semantics wrt how null values sort. |
|
@thruflo I think the problem is in your callback itself: ({ event }) => event.inserted_atYou're trying to access ({ event }) => event?.inserted_at |
|
Just trying to wrap my head around how event could be |
@thruflo could you console.log it just to confirm that |
|
Ok, I'm running the same code against this branch and it's now working, i.e.: I can use the inserted_at in the orderBy fine. |
18b597a to
feb9a17
Compare
Fixes #212 and fixes #229 by adding a configuration argument to
orderBy: