Skip to content

Configuration options for sortBy#314

Merged
kevin-dp merged 5 commits intomainfrom
kevindp/null-ordering
Aug 5, 2025
Merged

Configuration options for sortBy#314
kevin-dp merged 5 commits intomainfrom
kevindp/null-ordering

Conversation

@kevin-dp
Copy link
Copy Markdown
Contributor

@kevin-dp kevin-dp commented Jul 24, 2025

Fixes #212 and fixes #229 by adding a configuration argument to orderBy:

orderBy(
  callback: OrderByCallback<TContext>,
  options: OrderByDirection | OrderByOptions = `asc`
)

type OrderByOptions = {
  direction?: OrderByDirection
  nulls?: `first` | `last`
} & StringSortOpts

type StringSortOpts =
  | {
      stringSort?: `lexical`
    }
  | {
      stringSort?: `locale`
      locale?: string
      localeOptions?: object
    }

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jul 24, 2025

🦋 Changeset detected

Latest commit: feb9a17

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@tanstack/db Patch
@tanstack/electric-db-collection Patch
@tanstack/query-db-collection Patch
@tanstack/react-db Patch
@tanstack/solid-db Patch
@tanstack/svelte-db Patch
@tanstack/trailbase-db-collection Patch
@tanstack/vue-db Patch

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jul 24, 2025

More templates

@tanstack/db

npm i https://pkg.pr.new/@tanstack/db@314

@tanstack/db-ivm

npm i https://pkg.pr.new/@tanstack/db-ivm@314

@tanstack/electric-db-collection

npm i https://pkg.pr.new/@tanstack/electric-db-collection@314

@tanstack/query-db-collection

npm i https://pkg.pr.new/@tanstack/query-db-collection@314

@tanstack/react-db

npm i https://pkg.pr.new/@tanstack/react-db@314

@tanstack/solid-db

npm i https://pkg.pr.new/@tanstack/solid-db@314

@tanstack/svelte-db

npm i https://pkg.pr.new/@tanstack/svelte-db@314

@tanstack/trailbase-db-collection

npm i https://pkg.pr.new/@tanstack/trailbase-db-collection@314

@tanstack/vue-db

npm i https://pkg.pr.new/@tanstack/vue-db@314

commit: feb9a17

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 24, 2025

Size Change: +276 B (+0.47%)

Total Size: 58.5 kB

Filename Size Change
./packages/db/dist/esm/indexes/btree-index.js 1.47 kB +1 B (+0.07%)
./packages/db/dist/esm/query/builder/index.js 3.79 kB +122 B (+3.33%)
./packages/db/dist/esm/query/compiler/order-by.js 677 B -26 B (-3.7%)
./packages/db/dist/esm/utils/comparison.js 718 B +179 B (+33.21%) 🚨
ℹ️ View Unchanged
Filename Size
./packages/db/dist/esm/change-events.js 1.13 kB
./packages/db/dist/esm/collection.js 9.85 kB
./packages/db/dist/esm/deferred.js 230 B
./packages/db/dist/esm/errors.js 2.98 kB
./packages/db/dist/esm/index.js 1.51 kB
./packages/db/dist/esm/indexes/auto-index.js 689 B
./packages/db/dist/esm/indexes/base-index.js 605 B
./packages/db/dist/esm/indexes/lazy-index.js 1.25 kB
./packages/db/dist/esm/local-only.js 827 B
./packages/db/dist/esm/local-storage.js 2.03 kB
./packages/db/dist/esm/optimistic-action.js 294 B
./packages/db/dist/esm/proxy.js 4.19 kB
./packages/db/dist/esm/query/builder/functions.js 575 B
./packages/db/dist/esm/query/builder/ref-proxy.js 890 B
./packages/db/dist/esm/query/compiler/evaluators.js 1.48 kB
./packages/db/dist/esm/query/compiler/expressions.js 631 B
./packages/db/dist/esm/query/compiler/group-by.js 2.03 kB
./packages/db/dist/esm/query/compiler/index.js 1.74 kB
./packages/db/dist/esm/query/compiler/joins.js 1.56 kB
./packages/db/dist/esm/query/compiler/select.js 655 B
./packages/db/dist/esm/query/ir.js 318 B
./packages/db/dist/esm/query/live-query-collection.js 2.45 kB
./packages/db/dist/esm/query/optimizer.js 2.44 kB
./packages/db/dist/esm/SortedMap.js 1.24 kB
./packages/db/dist/esm/transactions.js 2.29 kB
./packages/db/dist/esm/utils.js 419 B
./packages/db/dist/esm/utils/btree.js 5.93 kB
./packages/db/dist/esm/utils/index-optimization.js 1.62 kB

compressed-size-action::db-package-size

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 24, 2025

Size Change: 0 B

Total Size: 1.05 kB

ℹ️ View Unchanged
Filename Size
./packages/react-db/dist/esm/index.js 152 B
./packages/react-db/dist/esm/useLiveQuery.js 902 B

compressed-size-action::react-db-package-size

@asabil
Copy link
Copy Markdown
Contributor

asabil commented Jul 24, 2025

Would it make sense to mimic PostgreSQL's default ordering?

By default, null values sort as if larger than any non-null value; that is, NULLS FIRST is the default for DESC order, and NULLS LAST otherwise.

@kevin-dp
Copy link
Copy Markdown
Contributor Author

Would it make sense to mimic PostgreSQL's default ordering?

By default, null values sort as if larger than any non-null value; that is, NULLS FIRST is the default for DESC order, and NULLS LAST otherwise.

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?

@samwillis
Copy link
Copy Markdown
Collaborator

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?

@kevin-dp
Copy link
Copy Markdown
Contributor Author

@samwillis agreed. Cleaner with a configuration object. I'm not sure about the union type OrderByDirection | OrderByOptions though. It's probably fine because most of the time one would only specify asc/desc order without specifying for nulls?

@KyleAMathews
Copy link
Copy Markdown
Collaborator

Yeah I like avoiding a third arg too. Union seems fine to me as yeah, 99% of time people will just specify the direction.

Copy link
Copy Markdown
Contributor

@thruflo thruflo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks 🙏

Comment thread packages/db/src/query/builder/index.ts Outdated
@thruflo
Copy link
Copy Markdown
Contributor

thruflo commented Jul 24, 2025

@thruflo
Copy link
Copy Markdown
Contributor

thruflo commented Jul 24, 2025

Also, whilst we're considering the orderBy signature #316

Copy link
Copy Markdown
Contributor

@thruflo thruflo left a 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:

  1. handle undefined fields / zod foo.optional()s
  2. 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.

@kevin-dp
Copy link
Copy Markdown
Contributor Author

@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.

@kevin-dp kevin-dp force-pushed the kevindp/null-ordering branch from 5d86583 to 52ecb77 Compare July 28, 2025 09:33
@kevin-dp kevin-dp requested review from samwillis and thruflo July 28, 2025 09:35
@kevin-dp
Copy link
Copy Markdown
Contributor Author

Would it make sense to mimic PostgreSQL's default ordering?

By default, null values sort as if larger than any non-null value; that is, NULLS FIRST is the default for DESC order, and NULLS LAST otherwise.

@asabil Since TanStack DB is backend-agnostic we decided to stick to JS semantics wrt how null values sort.

@kevin-dp kevin-dp changed the title Configurable sortBy order of null values Configuration options for sortBy Jul 28, 2025
@kevin-dp
Copy link
Copy Markdown
Contributor Author

kevin-dp commented Jul 28, 2025

@thruflo I think the problem is in your callback itself:

({ event }) => event.inserted_at

You're trying to access inserted_at but event is undefined. That's throwing the error you're seeing. Can you try replacing it by this:

({ event }) => event?.inserted_at

@thruflo
Copy link
Copy Markdown
Contributor

thruflo commented Jul 28, 2025

Just trying to wrap my head around how event could be undefined?

@kevin-dp
Copy link
Copy Markdown
Contributor Author

Just trying to wrap my head around how event could be undefined?

@thruflo could you console.log it just to confirm that event is undefined. Could be that we try to access inserted_at somewhere internally and that the error comes from there, but from the stack trace i got the impression it is the event that is undefined.

@thruflo
Copy link
Copy Markdown
Contributor

thruflo commented Jul 30, 2025

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.

@kevin-dp kevin-dp force-pushed the kevindp/null-ordering branch from 18b597a to feb9a17 Compare August 5, 2025 07:16
@kevin-dp kevin-dp merged commit 97b595e into main Aug 5, 2025
6 checks passed
@kevin-dp kevin-dp deleted the kevindp/null-ordering branch August 5, 2025 07:36
@github-actions github-actions Bot mentioned this pull request Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orderBy on dates works incorrectly when mixing formats Feature request: nulls first and custom comparator functions.

5 participants