Skip to content

feat: no-set-return-in-useOnyx-selector rule#179

Merged
mountiny merged 3 commits intoExpensify:mainfrom
callstack-internal:feat/no-set-return-in-useOnyx-selector
Feb 25, 2026
Merged

feat: no-set-return-in-useOnyx-selector rule#179
mountiny merged 3 commits intoExpensify:mainfrom
callstack-internal:feat/no-set-return-in-useOnyx-selector

Conversation

@MrMuzyk
Copy link
Contributor

@MrMuzyk MrMuzyk commented Feb 20, 2026

$ Expensify/App#82251

Summary

  • Add a new ESLint rule that disallows returning Set or ReadonlySet from
    useOnyx selectors. The deepEqual comparison used internally by useOnyx is
    extremely slow for Set objects, causing unnecessary performance degradation.
    The rule suggests returning an array instead and converting to a Set outside
    the selector if needed.
  • Extract shared AST utilities (findProperty, resolveVariable, getVariableInit,
    getVariableAsObject) into utils/astUtil.js, deduplicating identical helper
    functions that were previously inlined in no-inline-useOnyx-selector and
    provide-canBeMissing-in-useOnyx.

Detection coverage

The rule catches Set returns in the following patterns:

  • Inline arrow/function selectors (selector: (d) => new Set(d))
  • Variable-referenced selectors (const fn = (d) => new Set(d); ... {selector:
    fn})
  • useCallback-wrapped selectors
  • Options passed as a variable identifier
  • Variable tracing through intermediate assignments (const s = new Set(d);
    return s)
  • TypeScript return type annotations (: Set, : ReadonlySet)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@MrMuzyk
Copy link
Contributor Author

MrMuzyk commented Feb 23, 2026

I have read the CLA Document and I hereby sign the CLA

@MrMuzyk MrMuzyk marked this pull request as ready for review February 23, 2026 09:22
@MrMuzyk
Copy link
Contributor Author

MrMuzyk commented Feb 23, 2026

recheck

@mountiny mountiny self-requested a review February 23, 2026 16:35
CLABotify added a commit to Expensify/CLA that referenced this pull request Feb 23, 2026
@mountiny
Copy link
Contributor

Missing Test Coverage

Nice rule! Here are some test cases I think would strengthen the coverage:

Additional valid cases (should NOT error):

  1. Selector returns Array.from(new Set(...)) — result is an array, not a Set:

    selector: (d) => Array.from(new Set(d))
  2. Selector returns spread of Set — result is an array:

    selector: (d) => [...new Set(d)]
  3. new Set() used internally but array returned:

    selector: (d) => { const s = new Set(d); return [...s]; }
  4. Imported selector from another module (should not false-positive on unresolvable reference):

    import {mySelector} from './selectors';
    useOnyx(KEY, {selector: mySelector, canBeMissing: true});

Additional invalid cases (should error):

  1. Ternary with Set in implicit arrow return — currently NOT caught by the rule:

    selector: (d) => d ? new Set(d) : new Set()

    bodyReturnsNewSet does not recurse into ConditionalExpression consequent/alternate, so this pattern is silently missed. Either the rule should be updated to handle this, or a valid test should document it as a known limitation.

  2. Return statement with ternary containing Set — also NOT caught:

    selector: (d) => { return d ? new Set(d) : []; }

    Same root cause — isNewSetExpression is checked against ReturnStatement.argument directly, which is a ConditionalExpression, not a NewExpression.

Bonus considerations

  • Map/ReadonlyMapfast-equals deepEqual has the same O(n²) comparison behavior for Map as for Set. Worth considering whether the rule should also cover Map returns, or at least documenting the decision not to.

  • useMemo wrapped selectorsresolveSelectorFunction handles useCallback but not useMemo. While less common, const sel = useMemo(() => (d) => new Set(d), []) would not be caught.

@MrMuzyk
Copy link
Contributor Author

MrMuzyk commented Feb 24, 2026

I've updated the code to cover for missing cases and expanded the rule to include Map too

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Thank you!

@mountiny mountiny merged commit 12164a6 into Expensify:main Feb 25, 2026
6 checks passed
@os-botify
Copy link
Contributor

os-botify bot commented Feb 25, 2026

🚀 Published to npm in 2.0.105 🎉

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.

2 participants