-
Notifications
You must be signed in to change notification settings - Fork 77
Refactor select improving spread (...obj
) and enabling nested projection.
#389
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: ref-refactor
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b9a4d73 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: +1.36 kB (+2.32%) Total Size: 59.9 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 1.05 kB ℹ️ View Unchanged
|
const sentinelKey = `__SPREAD_SENTINEL__${spreadAlias}` | ||
select[sentinelKey] = toExpression(spreadAlias) // Use alias as a simple reference | ||
// Helper to ensure we have a BasicExpression/Aggregate for a value | ||
function toExpr(value: any): BasicExpression | Aggregate { |
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.
Let's not introduce nested functions here but extract them outside this function into the scope of this module. Same for the isPlainObject
andbuildNestedSelect
functions.
? T | ||
: TSelectObject[K] extends Ref<infer T> | undefined | ||
: StripInternalKeys<TSelectObject>[K] extends Ref<infer T> | undefined |
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.
Similar to my comments on #386, where are we checking for extends Ref<infer T> | undefined
if we know it doesn't extend Ref<infer T>
? I believe we can just check for extends undefined
here.
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.
I reviewed this PR in detail. The code quality should be improved, i left comments on how to do this.
? never // This is a RefProxy, handled above | ||
: ResultTypeFromSelect<TSelectObject[K]> // Recursive for nested objects | ||
: never | ||
: StripInternalKeys<TSelectObject>[K] extends string |
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.
The cases for string, number, boolean, null, and undefined can be combined:
StripInternalKeys<TSelectObject>[K] extends string | number | boolean | null | undefined
? StripInternalKeys<TSelectObject>[K]
: ...
const finalResults = (row as any).__select_results | ||
const raw = (row as any).__select_results | ||
const finalResults = | ||
raw instanceof ValClass |
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.
We could simplify the code here by extracting these checks into a predicate isValue
that checks whether its an instance of ValClass
or whether it's an objects with type === 'val'
.
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.
If we want we could even introduce an additional helper function unwrap
on top of that which will return raw.value
if it's a value class/object, and raw
otherwise.
@@ -264,7 +274,16 @@ export function compileQuery( | |||
const resultPipeline: ResultStream = pipeline.pipe( | |||
map(([key, row]) => { | |||
// Extract the final results from __select_results and return [key, [results, undefined]] | |||
const finalResults = (row as any).__select_results | |||
const raw = (row as any).__select_results | |||
const finalResults = |
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.
Don't duplicate it here. Let's use the helper function as i proposed.
@@ -320,7 +339,15 @@ function processFrom( | |||
const extractedInput = subQueryInput.pipe( | |||
map((data: any) => { | |||
const [key, [value, _orderByIndex]] = data | |||
return [key, value] as [unknown, any] | |||
// Unwrap Value expressions that might have leaked through as the entire row | |||
const unwrapped = |
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.
Same
@@ -16,67 +17,180 @@ export function processSelectToResults( | |||
select: Select, | |||
_allInputs: Record<string, KeyedStream> | |||
): NamespacedAndKeyedStream { |
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 function has grown too big. Let's extract the addFromObject
function outside of this function.
@@ -249,11 +249,11 @@ export function liveQueryCollectionOptions< | |||
|
|||
// Store the key of the result so that we can retrieve it in the | |||
// getKey function | |||
resultKeys.set(value, rawKey) | |||
resultKeys.set(value as unknown as object, rawKey) |
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 IDE is saying value
is of type any
. Why do an unsafe typecast if the value
is typed as any
anyway. Could as well keep it as any
.
@@ -374,7 +374,8 @@ export function liveQueryCollectionOptions< | |||
return { | |||
id, | |||
getKey: | |||
config.getKey || ((item) => resultKeys.get(item) as string | number), | |||
config.getKey || | |||
((item) => resultKeys.get(item as unknown as object) as string | number), |
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.
Item is of type TResult extends object
so why do we need to typecast it to an object?
@@ -760,15 +761,19 @@ function optimizeFromWithTracking( | |||
* { from: users, select: { id, name } } | |||
* ``` | |||
*/ | |||
function isSafeToPushIntoExistingSubquery(query: QueryIR): boolean { | |||
// Check for aggregates in SELECT clause | |||
function isSafeToPushIntoExistingSubquery( |
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.
I'm not a fan of functions that look like this:
if (pred)
return false
return true
We can just compose one big predicate and return that:
return !pred
Now, i understand that for this function a big negated predicate might hamper readability. Therefore, i would rewrite it as such:
function isSafeToPushIntoExistingSubquery(
query: QueryIR,
whereClause: BasicExpression<boolean>,
outerAlias: string
): boolean {
if (
// If the subquery has a SELECT clause, block pushdown when the WHERE references
// fields that are computed by the subquery's SELECT (non pass-through projections).
(
query.select &&
(selectHasAggregates(query.select) ||
whereReferencesComputedSelectFields(query.select, whereClause, outerAlias))
) ||
// Check for GROUP BY clause
(query.groupBy && query.groupBy.length > 0) ||
// Check for HAVING clause
(query.having && query.having.length > 0) ||
// Check for ORDER BY with LIMIT or OFFSET (dangerous combination)
(
query.orderBy && query.orderBy.length > 0 &&
(query.limit !== undefined || query.offset !== undefined)
) ||
// Check for functional variants that might have side effects
query.fnSelect ||
(query.fnWhere && query.fnWhere.length > 0) ||
(query.fnHaving && query.fnHaving.length > 0) ||
) {
return false
}
// If none of the unsafe conditions are present, it's safe to optimize
return true
}
Even better would be to introduce helper functions for each unsafe case and use those.
Then it would become even more readable:
return !(unsafeSelect(query) || unsafeGroupBy(query) || unsafeHaving(query) || unsafeOrderBy(query) || unsafeFnSelect(query))
} | ||
|
||
const refs: Array<PropRef> = [] | ||
function collectRefs(expr: any) { |
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.
Extract this function to the module level.
Shall we also make helper functions for the 2 for loops?
@@ -97,6 +97,7 @@ export function createRefProxy<T extends Record<string, any>>( | |||
if (prop === `__refProxy`) return true | |||
if (prop === `__path`) return path | |||
if (prop === `__type`) return undefined // Type is only for TypeScript inference | |||
if (prop === `__orderId`) return target.__orderId |
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 is __orderId
used for and where? I couldn't find any usage of this property in this PR.
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.
I meant to request changes instead of approving in my previous review.
This branch started as a fix for the broken types when using a spread in a select (#385), but identified a couple of problems:
closes #385
stacked on #388