feat(use-unit-destructuring): add new rule for destructured units

[Olovyannikov]

Problem: implicit subscriptions when forgot remove unused subscriptions inside react components
e.g.:

const { setValue } = useUnit({ 
  value: $store, // not used, but create unnecessary re-render
  setValue: event 
})

Why is this important?

Implicit subscriptions can lead to:

• Performance issues: unnecessary re-renders when unused stores update
• Hard-to-debug behavior: component re-renders for unclear reasons
• Memory leaks: subscriptions that are never cleaned up properly

Rule Details

This rule enforces that:

• All properties passed in an object to useUnit must be destructured to prevent implicit subscriptions;
• All elements passed in an array to useUnit must be destructured to prevent implicit subscriptions also.

[netlify]

✅ Deploy Preview for eslint-plugin ready!

Name	Link
🔨 Latest commit	aa98a41
🔍 Latest deploy log	https://app.netlify.com/projects/eslint-plugin/deploys/69db64521be4460008297b60
😎 Deploy Preview	https://deploy-preview-175--eslint-plugin.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[kireevmp]

@Olovyannikov Thanks for taking the time to submit this! The rule logic seems to be solid 👍

There are a couple of good opportunities to improve this though – I'd appreciate it if you can take a look at my comments

[kireevmp]

Manual traversal of all specifiers can be avoided given the following selector, which is widely accepted in many other rules

const selector = {
  import: `ImportDeclaration[source.value=${PACKAGE_NAME.react}] > ImportSpecifier[imported.name=useUnit]`,
}

This not only finds specific instances of useUnit aliases, but also supports other effector-react imports via regex such as effector-react/scope (deprecated but still used).

With this, the code reduces to

return {
  [selector.import]: (node: TSESTree.ImportSpecifier) => void importedAs.add(node.local.name),

[kireevmp]

Many of these checks can be delegated to esquery as discussed previously. It also allows us to select a specific subject (with ESLint via :has selector)

const selector = {
  variable: { shape: "VariableDeclarator[id.type=ObjectPattern]", list: "VariableDeclarator[id.type=ArrayPattern]" },
  call: "CallExpression.init[arguments.length=1][callee.type=Identifier]",
  arg: { shape: "ObjectExpression.arguments", list: "ArrayExpression.arguments" },
}

The above group of selectors allows detecting all two combinations via

return {
  [`${selector.variable.shape}:has(> ${selector.call}:has(${selector.arg.shape}))`]: (node: ShapeCall): void => { ... },
  [`${selector.variable.list}:has(> ${selector.call}:has(${selector.arg.list}))`]: (node: ListCall): void => { ... },
}

And these selectors provide the following TS guarantees

type ListCall = Node.VariableDeclarator & {
  init: Node.CallExpression & { callee: Node.Identifier; arguments: [Node.ArrayExpression] }
  id: Node.ArrayPattern
}

type ShapeCall = Node.VariableDeclarator & {
  init: Node.CallExpression & { callee: Node.Identifier; arguments: [Node.ObjectExpression] }
  id: Node.ObjectPattern
}

Which completely eliminates the need for all guards except importedAs.has

[kireevmp]

This function can be moved out of create to be static (rather than re-created on each file) and also simplified by type-guarding against computed first

type ShapeProperty = Node.Property | Node.RestElement | Node.ObjectLiteralElement

function toKey(prop: ShapeProperty): string | number | null {
  if (prop.type !== NodeType.Property || prop.computed) return null
  if (prop.key.type === NodeType.Identifier) return prop.key.name
  if (prop.key.type === NodeType.Literal) return prop.key.value
  return null
}

[kireevmp]

1. This function does not capture anything from context as well, so can be moved out.
2. I don't see a specific reason to work with both argument and destructured keys in the same function, given that the logic is the same. It would be best to reuse the same logic by calling it twice, separately on lefthand and righthand side.
3. We should also build in the rest/spread logic by bailing early on any "weird" usage where we can't track what's provided to useUnit and what's consumed by the component.
4. We can also postpone keyToName generation until we've found the violation and ready to report

The shape -> key mapper can then be simplified to work along these lines

type ValueNode = Node.Property["key"]

function shapeToKeyMap(shape: Node.ObjectPattern | Node.ObjectExpression): Map<string | number, ValueNode> | null {
  const map = new Map<string | number, ValueNode>()

  for (const prop of shape.properties) {
    const key = toKey(prop)

    if (key === null || prop.type === NodeType.RestElement || prop.type === NodeType.SpreadElement)
      return null /* can't see what is consumed/provided, so skip to reduce false positives */
    else map.set(key, prop.key)
  }

  return map
}

This is called twice, first for ObjectPattern and then for ObjectExpression. The prop.key is stored for later to allow for formatting the error, but is untouched until that is needed.

[kireevmp]

1. Same thoughts here – no context capture suggests moving out of create and working with both lefthand and righthand sides makes no sense when we can call the function twice.
2. context.sourceCode.getText(node) is dependent on source code formatting and is not reliable, since it may produce "new line" character or other stuff we're not ready to work with. One other rule currently handles this by traversing the MemberExpression and joining the path recursively.
3. This logic should also consider bailing on spread and rest elements, since those completely remove our ability to judge what's provided and consumed.

With these thoughts, consider the following rough implementation

type ValueNode = Exclude<Node.DestructuringPattern, Node.RestElement> | null

function listToKeyMap(list: Node.ArrayPattern | Node.ArrayExpression): Map<number, ValueNode> | null {
  const map = new Map<number, ValueNode>()

  for (const [index, element] of list.elements.entries())
    if (element?.type === NodeType.RestElement || element?.type === NodeType.SpreadElement) return null
    else if (element === null) continue /* leave a hole as non-consuming */
    else map.set(index, element)

  return map
}

[kireevmp]

Every test case in this repo uses ts or tsx helper from "@/shared/tag" which trims spaces and newlines. It also works really nice with prettier-plugin-embed to provide automatic formatting (i.e. remove semicolons).

For intended cases like quoted "setValue" use // prettier-ignore

[kireevmp]

Would you also kindly sort this rule list alphabetically, like with the imports?

[kireevmp]

@Olovyannikov Thank you for committing your time to polish this PR! I think these are just final cosmetic touches on code location and consistency. Once fixed I believe we'll be in good shape to merge.

[kireevmp]

Suggested change

	description: Ensures that all units passed to useUnit are properly destructured to avoid unused subscriptions and implicit re-renders.
	description: Ensure all units passed to useUnit are properly destructured to avoid unused subscriptions and implicit re-renders

Just a small touch-up to remove the dot and use correct form.

[kireevmp]

Suggested change

	unusedKey: 'Property "{{key}}" is passed but not destructured.',
	missingKey: 'Property "{{key}}" is destructured but not passed in the unit object.',
	unusedKey: 'Property "{{name}}" is passed but not destructured.',
	missingKey: 'Property "{{name}}" is destructured but not passed in the unit object.',

Very much optional, but this will remove the data: { key: name } in favor of data: { name } shorthand.

[kireevmp]

Is there a reason to keep those handle* in separate functions, or would it be possible to just inline those in the visitor?

[kireevmp]

A question on rule naming – why were you inclined to change the previous name useUnit-destructuring and add prefer-?

In case with prefer-useUnit, the community prefers useUnit over useStore and others.

With these rule, I believe there's nothing to prefer. Rather, if a rule name prefix is wanted, I'd suggest ensure-useUnit-destructuring becase we ensure all units are destructured. What do you think? Which option you think is best here?

[kireevmp]

Why are these new rule-specific functions placed in a shared file that explicitly only houses createRule? These are very rule-specific, so I'd be inclined to keep them in the test rule file.

[kireevmp]

toKey already covers the RestElement and SpreadElement checks via prop.type !== AST_NODE_TYPES.Property. So in my previous comment with the rough implementation the if was solely as a type guard.

key acts as the discriminator in this case. Consider either type cast (which is sound) or other way to force TS to narrow down the value.

for (const prop of shape.properties) {
  const key = toKey(prop)

  if (key === null) return null
  else map.set(key, (prop as Node.Property).key)
}

There's also an option to replace Rest/SpreadElement check with the prop.type !== AST_NODE_TYPES.Property check inside the loop, removing it from toKey and narrowing down the argument type of toKey to reflect that.

[kireevmp]

Might make sense to reverse check to get rid of .? operator

Suggested change

	for (const [index, element] of list.elements.entries()) {
	if (element?.type === AST_NODE_TYPES.RestElement || element?.type === AST_NODE_TYPES.SpreadElement) return null
	if (element === null) continue
	for (const [index, element] of list.elements.entries()) {
	if (element === null) continue
	if (element.type === AST_NODE_TYPES.RestElement || element.type === AST_NODE_TYPES.SpreadElement) return null

[kireevmp]

This new code in this file is also very rule-specific, consider moving back to the rule file.

[kireevmp]

Suggested change

	description: "Ensure destructured properties match the passed unit object/array",
	description: "Ensure all units passed to useUnit are properly destructured.",

It's best to keep this very close to frontmatter in docs, and also mention useUnit.

[kireevmp]

It's rather blunt to ignore all XML files, even though we don't use them at the moment. Can this be more specific, and preferably under a named section like ignores below?