-
Notifications
You must be signed in to change notification settings - Fork 55
Configuration options for sortBy #314
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 18b597a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
@tanstack/db-example-react-todo • @tanstack/db-example-solid-todo @tanstack/db
@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: +277 B (+0.48%) Total Size: 58.4 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. |
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, thanks 🙏
Also, whilst we're considering the orderBy signature #316 |
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.
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_at You'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. |
Fixes #212 and fixes #229 by adding a configuration argument to
orderBy
: