mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 08:03:20 +00:00
perf(store): follow a selector one hop into its helper
Review feedback: both the lint rule and the manual sweep it was checked against only looked at the inline selector body, so neither could see a fresh allocation made inside a helper the selector calls — and delegating to a module-scope helper is the idiomatic shape here. Two methods sharing a blind spot is not corroboration. The two fresh-reference rules now resolve a single hop into a module-scope helper. The predicate used across that hop is deliberately stricter than the inline one: it requires EVERY returned expression to allocate unconditionally, so the common `cache.get(k) ?? buildFresh(state)` identity-caching shape is not flagged. An unresolvable helper is left alone rather than guessed at. Still zero hits across 20,330 files, so this stays a ratchet.
This commit is contained in:
@@ -173,6 +173,45 @@ function resolveSelector(argument, state) {
|
||||
return name ? (state.namedSelectors.get(name) ?? null) : null
|
||||
}
|
||||
|
||||
/**
|
||||
* Allocation that happens on EVERY call, used when following a selector into a
|
||||
* helper. Deliberately stricter than isAllocatingExpression: a helper that returns
|
||||
* a cached reference on one branch and builds a fresh one on another is the normal
|
||||
* identity-caching shape, and flagging it would be a false positive.
|
||||
*/
|
||||
function alwaysAllocates(expression) {
|
||||
if (expression?.type === 'ConditionalExpression') {
|
||||
return alwaysAllocates(expression.consequent) && alwaysAllocates(expression.alternate)
|
||||
}
|
||||
if (expression?.type === 'LogicalExpression') {
|
||||
return alwaysAllocates(expression.left) && alwaysAllocates(expression.right)
|
||||
}
|
||||
return (
|
||||
expression?.type === 'ArrayExpression' ||
|
||||
expression?.type === 'ObjectExpression' ||
|
||||
expression?.type === 'NewExpression' ||
|
||||
(expression?.type === 'CallExpression' &&
|
||||
ALLOCATING_METHODS.has(propertyName(expression.callee) ?? ''))
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* One hop: a selector that delegates to a module-scope helper is the idiomatic
|
||||
* shape here, and neither the inline-body check nor a reviewer reading the call
|
||||
* site can see what that helper returns.
|
||||
*/
|
||||
function expandThroughNamedHelper(expression, state) {
|
||||
if (expression?.type !== 'CallExpression') {
|
||||
return [expression]
|
||||
}
|
||||
const helper = state.namedSelectors.get(identifierName(expression.callee) ?? '')
|
||||
if (!helper) {
|
||||
return [expression]
|
||||
}
|
||||
const returned = returnedExpressions(helper)
|
||||
return returned.length > 0 && returned.every(alwaysAllocates) ? returned : [expression]
|
||||
}
|
||||
|
||||
function createRuleState() {
|
||||
return {
|
||||
appStoreHooks: new Set(),
|
||||
@@ -270,7 +309,8 @@ function deferredSelectorRule(inspect) {
|
||||
selector: resolveSelector(argument, state),
|
||||
argument,
|
||||
shallow,
|
||||
node
|
||||
node,
|
||||
state
|
||||
})
|
||||
if (report) {
|
||||
this.report(report)
|
||||
@@ -293,11 +333,13 @@ function noIdentitySelectorRule() {
|
||||
}
|
||||
|
||||
function noFreshSelectorResultRule() {
|
||||
return deferredSelectorRule(({ selector, shallow }) => {
|
||||
return deferredSelectorRule(({ selector, shallow, state }) => {
|
||||
if (shallow || !selector) {
|
||||
return null
|
||||
}
|
||||
const freshResult = returnedExpressions(selector).find(isAllocatingExpression)
|
||||
const freshResult = returnedExpressions(selector)
|
||||
.flatMap((expression) => expandThroughNamedHelper(expression, state))
|
||||
.find(isAllocatingExpression)
|
||||
return freshResult
|
||||
? {
|
||||
node: freshResult,
|
||||
@@ -322,12 +364,14 @@ function nestedFreshValues(expression) {
|
||||
}
|
||||
|
||||
function noNestedFreshUnderShallowRule() {
|
||||
return deferredSelectorRule(({ selector, shallow }) => {
|
||||
return deferredSelectorRule(({ selector, shallow, state }) => {
|
||||
if (!shallow || !selector) {
|
||||
return null
|
||||
}
|
||||
const nestedFresh = returnedExpressions(selector)
|
||||
.flatMap((expression) => expandThroughNamedHelper(expression, state))
|
||||
.flatMap(nestedFreshValues)
|
||||
.flatMap((expression) => expandThroughNamedHelper(expression, state))
|
||||
.find(isAllocatingExpression)
|
||||
return nestedFresh
|
||||
? {
|
||||
|
||||
@@ -97,4 +97,34 @@ describe('app store performance Oxlint plugin', () => {
|
||||
'app-store-performance(no-nested-fresh-under-shallow)'
|
||||
])
|
||||
})
|
||||
|
||||
it('follows a selector one hop into a module-scope helper', () => {
|
||||
const diagnostics = lintSource(`
|
||||
import { useAppStore } from '@/store'
|
||||
import { useShallow } from 'zustand/react/shallow'
|
||||
const buildRows = (state) => state.rows.map((row) => row.id)
|
||||
const Delegating = () => useAppStore((state) => buildRows(state))
|
||||
const NestedDelegating = () => useAppStore(useShallow((state) => ({ ids: buildRows(state) })))
|
||||
`)
|
||||
|
||||
expect(diagnostics.map((diagnostic) => diagnostic.code)).toEqual([
|
||||
'app-store-performance(no-fresh-selector-result)',
|
||||
'app-store-performance(no-nested-fresh-under-shallow)'
|
||||
])
|
||||
})
|
||||
|
||||
it('does not flag a helper that returns a cached reference on some branch', () => {
|
||||
const diagnostics = lintSource(`
|
||||
import { useAppStore } from '@/store'
|
||||
import { useShallow } from 'zustand/react/shallow'
|
||||
// The identity-caching shape: fresh only on a miss, cached otherwise.
|
||||
const selectCachedRows = (state) => cache.get(state.key) ?? state.rows.filter(Boolean)
|
||||
const Cached = () => useAppStore((state) => selectCachedRows(state))
|
||||
const CachedNested = () => useAppStore(useShallow((state) => ({ rows: selectCachedRows(state) })))
|
||||
// An unknown helper cannot be resolved, so it must not be guessed at.
|
||||
const External = () => useAppStore((state) => externalBuild(state))
|
||||
`)
|
||||
|
||||
expect(diagnostics).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user